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

Consider changing output type of ArgMax/Argmin to int32, or allow passing output_type #653

Closed
philloooo opened this issue Apr 23, 2024 · 2 comments · Fixed by #730
Closed

Comments

@philloooo
Copy link
Contributor

philloooo commented Apr 23, 2024

Currently WebNN specifies ArgMax/Min returns int64.

  • CoreML supported return type is int32 or uint16.
  • tflite supports int32, int64.
  • dml supports int32, int64, uint32, uint64.

Returning int64 can't be emulated on CoreML:

  • the cast operator only supports casting to fp16, fp32, i32, bool.
  • None of the operations support int64 as input(so the output of argMax/argMin can't be connected to the next layer if it's int64)
  • Graph output doesn't support i64. Only fp16, fp32, i32 is supported.

Given int32 is the intersection across these backends, I see two options to make it work:
a. Update argMin/Max to always output int32
b. On the spec level, allow passing a param of output_type. And allow probing the supported data types for this param.

@philloooo philloooo changed the title Consider changing output type of ArgMax/Argmin to int32, or allow passing output_type for ArgMax/ArgMin Consider changing output type of ArgMax/Argmin to int32, or allow passing output_type Apr 23, 2024
@philloooo
Copy link
Contributor Author

Following up from the working group meeting, I am happy to explore option 2 if @fdwr can provide examples on when int64 is useful.

I didn't fully understand your gather example, because I actually don't understand why indices allow both int64 and uint32. The indices should point to valid indices that's within MLOperand's dimensions right? And the dimensions are uint32...

@fdwr
Copy link
Collaborator

fdwr commented Jul 18, 2024

Mirroring this table here too for historical reference:

API uint32 int32 uint64 int64 Link
DML uint32 int32 uint64 int64 DML_ARGMIN_OPERATOR_DESC
TF output_type - int32 - int64 tf.math.argmin
CoreML - int32 - - reduction.reduce_argmin
MLIR TOSA uint32 - - - tosa.argmax "signless integer"
ONNX - - - int64 ArgMin
PyTorch - - - int64 torch.argmin
NumPy - - - int64 numpy.argmin

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

Successfully merging a pull request may close this issue.

4 participants