Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JulienSiems
Copy link

This PR adds the argmax operator to wonnx.

@JulienSiems JulienSiems changed the title Add argmax Add argmax operator. Jul 18, 2023
Copy link
Collaborator

@pixelspark pixelspark left a 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());
Copy link
Collaborator

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).

Copy link
Author

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);
Copy link
Collaborator

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.

Copy link
Author

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
Copy link
Collaborator

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)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants