-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Support for negative indices in 'Gather', 'GatherElements', 'ScatterElements', 'OneHot' #2260
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.
Some inline comments
Should I make a similar change for Gather_Nd. Alternatively, could you please include Gather_Nd when the op gets in (hopefully tomorrow) in this PR? That way all the negative indices spec updates can go in together ? :) |
@hariharans29 I can add GatherND in this PR |
Thanks @neginraoof. I actually had to make some formatting changes in the doc, so I went ahead and made the Gather_ND spec support negative indices (based on your change here). As part of your change, I think you can refine the sentence in GathertND if you want. Thanks! |
…gativeIndices # Conflicts: # docs/Operators.md # onnx/defs/operator_sets.h
@hariharans29 Thanks, I just rebased. The docs are consistent now. |
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.
Thank you so much for this great work! Overall LGTM. Just make sure my last minor comments are addressed.
Co-Authored-By: Wei-Sheng Chin <wschin@outlook.com>
Co-Authored-By: Wei-Sheng Chin <wschin@outlook.com>
@wschin Sure. I updated the PR as requested. |
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.
LGTM. Added a couple of minor comments.
onnx/defs/tensor/old.cc
Outdated
fail_type_inference("OneHot node must have three inputs."); | ||
} | ||
// Input 'depth' must be a scalar or a single-element vector. | ||
// TODO: Ideally to match spec for this input only allow Scalar should be allowed. |
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.
This sentence is not phrased well. We should correct it.
cc @gramalingam @ebarsoum please review |
…gativeIndices # Conflicts: # docs/Changelog.md # docs/Operators.md # docs/TestCoverage.md # onnx/backend/test/case/node/onehot.py # onnx/defs/operator_sets.h # onnx/defs/tensor/defs.cc # onnx/defs/tensor/old.cc
…lements', 'OneHot' (onnx#2260) * modified gather docs to support negative indices * added support for negatice indices to gather_elements and scatter_elements * fixed documentation formatting as per comments * Added negatice indices to docs for OneHot op * GatheND spec for negative indices editted * fix for comments * fixed formatting for gather op * Update onnx/defs/tensor/defs.cc Co-Authored-By: Wei-Sheng Chin <wschin@outlook.com> * Update docs/Changelog.md Co-Authored-By: Wei-Sheng Chin <wschin@outlook.com> * added print examples to onehot and gather as per comments * adding modifies model tests for onehot * updated doc files * updating unsqueeze test * typo fix as per comments
We would need to support negative indices for these operators, since PyTorch does support this case. Using Select/Gather ops with -1 index is quite common among PyTorch models.