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

onnx: ReduceMin/Max Ops #2563

Merged
merged 13 commits into from
Oct 15, 2024
Merged

Conversation

AnubhabB
Copy link
Contributor

@AnubhabB AnubhabB commented Oct 14, 2024

Adds support for ReduceMin and ReduceMax Ops. Found these in use in a Detectron2 exported model.

Implementation
Supports Ops version 18+ Backward compatible as far as I can tell

Open Questions

  • Backward compatibility: Op version 13 -> 18 has moved the axes from attributes to input. Though it's trivial to handle this with get_attr_opt::<&[i64]>(node, "axes") fallback, not sure if that's the right way of going about this!
  • The Op definition allows for empty set as input and prescribes -INF/ +INF or Datatype::Min/ Datatype::Max for the ops. Reference. I'm not sure if the return is supposed to be a scalar?

@LaurentMazare, any thoughts around these open questions?

@LaurentMazare
Copy link
Collaborator

  • Backward compatibility: Op version 13 -> 18 has moved the axes from attributes to input. Though it's trivial to handle this with get_attr_opt::<&[i64]>(node, "axes") fallback, not sure if that's the right way of going about this!

Falling back to taking the axis from the input if they are not present in the attributes sounds pretty reasonable to me.

  • The Op definition allows for empty set as input and prescribes -INF/ +INF or Datatype::Min/ Datatype::Max for the ops. Reference. I'm not sure if the return is supposed to be a scalar?

I guess we can just error out in this case for the moment, and if anyone runs into this issue then we can have a look at what the failing onnx model actually expects.

@AnubhabB
Copy link
Contributor Author

Thanks @LaurentMazare.

I've amended the PR with the following:

  1. Fetching axes defaults to the current standard (via input) and falls back to attribute for backward compatibility.
  2. Errors out on empty Tensors
  3. Tests for backward compatibility and empty set error.

Guess this is good to go!

@LaurentMazare LaurentMazare merged commit a01aa89 into huggingface:main Oct 15, 2024
10 checks passed
@LaurentMazare
Copy link
Collaborator

Thanks!

@AnubhabB AnubhabB deleted the onnx-reduce-min-max branch October 15, 2024 08:39
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.

2 participants