-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add argmax operator. #175
base: master
Are you sure you want to change the base?
Add argmax operator. #175
Conversation
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.
Great contribution! Just a few notes, should be fine to merge after.
@@ -40,7 +40,9 @@ fn main(@builtin(global_invocation_id) global_id: vec3<u32>) { | |||
Now for each reduced axis, iterate all values and reduce. Note, starting value may not always be zero. For | |||
ReduceMin/Max we should initialize as NaN and keep a flag to check if we have seen at least one element -#} | |||
|
|||
var accumulator = {% if op_type == "ReduceProd" %} {{ scalar_type }}(1) {% else %} Scalar() {% endif %}; | |||
var accumulator = {% if op_type == "ReduceProd" %} {{ scalar_type }}(1) {% else %} Scalar() {% endif %}; | |||
var max_element: Scalar = log(Scalar()); |
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.
Hm, perhaps add a note explaining why you initialize to log(Scalar())
(I assume it is a trick to initialize to -Inf
so the first value is always higher).
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.
Good point, will do!
{% elif op_type == "ArgMax" %} | ||
if(input_val > max_element) { | ||
max_element = input_val; | ||
accumulator = f32(count); |
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.
The output of ArgMax
is actually specified as int64
(see here). This should work for now but we might have to fix it later.
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.
Good point, then it would probably make sense to move argmax out of reduce to not have to return mixed data types from reduce?
@@ -223,6 +235,17 @@ fn reduce() { | |||
&[3, 2], | |||
); | |||
|
|||
// ONNX test case: do_not_keepdims with ArgMax |
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.
Can you also enable the corresponding test case in the ONNX backend test (Python scripts)?
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.
Will do!
This PR adds the argmax operator to wonnx.