I know it is about trade-off. I am suggesting a trade-off , how many apis do
we have that takes too many parameters ?
From what I recall its around 20. Why can we not create the builder just for
these APIs( which we discussed), why is it necessary to add 200 Apis ?
Are you suggesting to create
Kellen,
Could you please explain why you think range loops are better and how it
improves readability? this is a relatively new feature, many of them are
used to the old syntax, shouldn't we leave it for the developers to choose
the one that best suits the need and their familiarity.
In general
Naveen, software designing is all about tradeoff, every feature we
introduce causes more compiling time, more efforts to maintain, etc.
The main difference is.
Option #1: Java users do
NDArray.BatchNorm(data, gamma, beta, null, null, null, null, null,
null, null, null, null, null, null);
(and
It's more readable because it's concise and it's consistent for many types
you're looping over (i.e. primitive arrays, stl iterators, etc all work the
same way). It's also useful because it's consistent with other programming
languages, making C++ codebases much easier to read for novice and
while other PRs are all good.
On Sat, Sep 29, 2018 at 2:13 PM YiZhi Liu wrote:
>
> Honestly I don't know yet. I can help to investigate. Just given the
> evidence that, travis timeout every time it gets re-triggered - 2
> times at least. Correct me if I'm wrong @ Zhennan
> On Sat, Sep 29, 2018 at
Reading over the PR I don't see what aspects would cause extra runtime
YiZhi, could you point them out?
On Sat, Sep 29, 2018 at 8:46 PM YiZhi Liu wrote:
> Kellen, I think this PR introduces extra runtime in CI, thus causes
> the timeout. Which means, once merged, every PR later will see same
>
Honestly I don't know yet. I can help to investigate. Just given the
evidence that, travis timeout every time it gets re-triggered - 2
times at least. Correct me if I'm wrong @ Zhennan
On Sat, Sep 29, 2018 at 1:54 PM kellen sunderland
wrote:
>
> Reading over the PR I don't see what aspects would
+1
Maybe it's not necessary to enforce usage of range-based for, but I would
highly encourage to to it due to already named advantages. If code would be
introduced using the old-style there could be a comment suggesting the new
way. But why do the manual work and not leave that to the automated
Hi YiZhi and Kellen,
From my point of view, travis should be able to get passed from a scratch
build. Pending result on ccache hit/miss is not a good idea. For this PR, as it
changed many header file, lots of files need be recompiled, just like a scratch
build. I think that's the reason that
Hey Zhennan, yes this is the exact problem, and I agree with your points
completely. This is why when we first added Travis we attempted to
communicate that it would be informational only, and that we'd need to
iterate on the config before it would be a test that people should consider
Could you upload the picture somewhere please? HTML is being stripped out
on email lists.
Chiyuan Zhang schrieb am So., 30. Sep. 2018, 03:44:
> There is an option in the repo settings menu to disable or enable
> merge-commit for PR, see a screenshot below (from a different github
> project):
>
yes, AFAIK I think Apache Infra has disabled the merge option.
If there is a valid reason(and this is one), we could ask our Mentors to
help us create a INFRA ticket to temporarily enable this option and once we
are done merging we can request to disable it again.
On Sat, Sep 29, 2018 at 9:44 PM
I think we have had enough of an debate between the two of us and I have
already listed my reasons, I will stop here and see what others say given
my reasoning.
-1 to #2)
Also, by lecture I meant to say "I don't want to list all the problems
with unnecessary complications and talk about how to
Ah! we agree on something :) lets get more opinions, I am happy to go with
it.
On Sat, Sep 29, 2018 at 10:40 PM YiZhi Liu wrote:
> Also sometimes people may not be at the same page when talking about option
> #2. What I insist is the builder classes for each operator. Otherwise I
> actually
I am not sure what makes you think that I am suggesting we should not fix
it. I am pointing out that many of those are incorrectly optional so don't
take that into consideration to make a decision whether we need builders
for all.
On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu wrote:
> And if we
IMO, its unnecessary. this is only my opinion and you are free to disagree.
With more opinions one or some of us will have to commit to the majority of
the opinion.
On Sat, Sep 29, 2018 at 10:35 PM YiZhi Liu wrote:
> No you haven't answered my question "Since you agree to have 30+
> operators
Yes agreement and disagreement stay at technical level only:)
Back to the problem, they are unnecessary but good in terms of,
1. Still not good for java users to write 3 nulls in a function call with 5
or 4 args
2. Every function call with a “tail” null for arg “out”. I would say, makes
it seems
And if you want a more authoritative opinion on that check out what the C++
core guidelines are saying [1]:
> ES.71: Prefer a range-for-statement to a for-statement when there is a
choice
> Reason
> Readability. Error prevention. Efficiency.
Best regards
Anton
[1]
There is an option in the repo settings menu to disable or enable
merge-commit for PR, see a screenshot below (from a different github
project):
[image: image.png]
My guess is that this is disabled for the reason to avoid creating
non-linear history for standard PRs (as oppose to technical
This makes sense. Thanks
On Sat, Sep 29, 2018 at 6:36 PM kellen sunderland <
kellen.sunderl...@gmail.com> wrote:
> Hey Zhennan, yes this is the exact problem, and I agree with your points
> completely. This is why when we first added Travis we attempted to
> communicate that it would be
We can also request Infra directly to execute an override action on our
behalf - that way, they don't have to change the configuration and it
creates less work for them. That's something they did for us back then when
we switched CI. If we are all aligned, it shouldn't be a big deal for them
to
Also sometimes people may not be at the same page when talking about option
#2. What I insist is the builder classes for each operator. Otherwise I
actually more support Naveen’s approach - not to totally separate java and
scala objects.
On Sat, Sep 29, 2018 at 7:35 PM YiZhi Liu wrote:
> No you
Well, I am not sure(I don't think) we need Builder for every API in
NDArray. For APIs that take long list of parameters, I agree to add Builder.
Look at the API distribution based on number of arguments here:
https://gist.github.com/nswamy/2dea72e514cc7bfc675f68aef9fe78bb
about 30 APIs have 7 or
Thanks Kellen & Anton, for your detailed explanation and links to
advantages, appreciate it.
changing my vote to *-0*, I suggest to show as warnings.
On Sat, Sep 29, 2018 at 8:06 PM Anton Chernov wrote:
> And if you want a more authoritative opinion on that check out what the C++
> core
Sorry, here is the image: https://imgur.com/V5wd2XB
And here is the github document on the 3 different merge options for the
web UI button: https://help.github.com/articles/about-pull-request-merges/
On Sat, Sep 29, 2018 at 6:48 PM Marco de Abreu
wrote:
> Could you upload the picture somewhere
And if we find incorrect declaration, we fix it, not simply assuming
many of them also has problem and we cannot rely on them - otherwise
the type-safe APIs in Scala also does not make sense.
On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu wrote:
>
> It also makes sense to me if we have it under
How many errors exist in the code base right now if it were to be enabled?
On Sat, Sep 29, 2018 at 7:03 PM Naveen Swamy wrote:
> Thanks Kellen & Anton, for your detailed explanation and links to
> advantages, appreciate it.
> changing my vote to *-0*, I suggest to show as warnings.
>
> On Sat,
Java APIs are not like Clojure - The current proposal is only to build a
few thin wrappers for Inference.
To better represent the two cases and this discussion in particular, here
is an example API
1) def Activation (data : org.apache.mxnet.NDArray, act_type : String, out
: Option[NDArray] =
Hi, I would like to subscribe to the ASF mxnet channel.
*Cosmin Catalin SANDA*
Data Scientist & Engineer
Phone: +45.27.30.60.35
Web: https://cosminsanda.com
Invite sent. Welcome to Apache MXNet Cosmin :).
On Sat, Sep 29, 2018 at 11:38 AM Cosmin Cătălin Sanda <
cosmincata...@gmail.com> wrote:
> Hi, I would like to subscribe to the ASF mxnet channel.
>
> *Cosmin Catalin SANDA*
> Data Scientist & Engineer
> Phone:
Kellen, I think this PR introduces extra runtime in CI, thus causes
the timeout. Which means, once merged, every PR later will see same
timeout in travis.
So shall we modify the changes to decrease the test running time? or
just disable the Travis CI?
On Fri, Sep 28, 2018 at 9:17 PM Qin,
Moreover, regarding "add 200+ APIs" - this was exactly what we did for
type-safe APIs in Scala, we have NDArray/Symbol.BatchNorm and
NDArray/Symbol.api.BatchNorm - why did we decide to do that? Because
we thought type-safe could provide better user-experience. Given the
example I listed above, I
Some of my comments inline:
> Why can we not create the builder just for these APIs( which we discussed),
> why is it necessary to add 200 Apis
It is about unified user-experience. And we get rid of annoying extra
"out=null" in every operator.
> Are you suggesting to create builder for each and
33 matches
Mail list logo
