Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement argMax and argMin #62
Implement argMax and argMin #62
Changes from 2 commits
9856a81
cf4cae7
ff919e5
857871f
9321913
7fe0842
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding
axes ??
, I'm not that familiar with the falsesyness of JS evaluation. If you passaxes = []
, will that correctly use the explicit emptyaxes
(output
shape is then the same size as theinput
, and there is a 1:1 mapping of output indices) rather than generate full reduction?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so very special, it's neither null nor a sequence having values in the range [0, N-1]. Is there any model using this axes?
According to https://webmachinelearning.github.io/webnn/#dom-mlargminmaxoptions-axes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My argument is mainly driven by robustness plus consistency with similar behavior for the reduction operators when passed explicit
[]
axes. You probably won't find a model unless you look really hard, but some model generation parameters could programmatically produce it; and we see that in related reduction operators like ReduceSum that explicit empty axes[]
was important enough that ONNX specifically added thenoop_with_empty_axes
attribute later https://onnx.ai/onnx/operators/onnx__ReduceSum.html. Speaking of that, do we have any explicit axes =[]
cases inreduceSum
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last we also missed considering this axes = [] cases in reduceSum.
Since current Spec is lack of explanation for axes = [], I will open an issue for discussing it on WG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty, and I will add my clear expectation of what should happen in the nop case 😉 (especially if WebNN is to support existing frameworks behavior where
[]
means nop).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to webmachinelearning/webnn#493
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was fast! I tried ONNX and TF:
(will add that to the issue...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to wait for the spec discussion or proceed and react later accordingly? (I don't mind either way, and you might not get a reply for the next 2 weeks...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will follow your guide to support axes=[] here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.s. Appears that the Chromium implementation supports explicit
axes = []
gracefully:https://chromium-review.googlesource.com/c/chromium/src/+/5125652
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appears this will fail on the
axes = []
case, which should return exactly one element of shape[]
. Most of the logic already looks correct to handle that case (includingsizeOfShape
which will return 1).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange. It can work for this case without changes, my understanding is that index should be start from 0.
But I add
if
condition block before this, then it never goes here for axes=[], please see the update commit, thanks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(huh, what a weird language: https://stackoverflow.com/questions/58570277/adding-an-element-in-negative-index-of-an-array)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduceStrides[-1] = 1
getting[-1: 1]
is bad, we should avoid that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @huningxin!
I've updated commit to avoid it, please take another look, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
reducedElementCount
?reduceElements
sounds like it's actually the elements themselves (the values).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.