-
Notifications
You must be signed in to change notification settings - Fork 382
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 support for scalar unsqueeze #1690
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.
I'm somewhat ambivalent about these changes. Obviously, to support unsqueeze for scalars right now we have to do something like this.
But at the same time I feel like this is gonna come back to haunt us for other reasons 😅
This is just very isolated, and with the state of other ops that have different behavior for scalars (e.g., some just capture the value in a 1D tensor with a single element).
As an isolated change to support unsqueeze for scalars, this is fine. I'll leave my review as a comment for now, interested in hearing feedback from others (@nathanielsimard and yourself included)
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.
Same comment as @laggui, I think this is fine in the short term, but supporting scalar tensors from onnx without having to sync the device is very valuable, we'll have to think about how we can do it easily.
* Revert 1c639c8 1c639c8?diff=unified&w=0 * Refactor by @laggui * Refactor unsqueeze * Add support for scalar unsqueeze * Removed dead comment
There is case when we need to unsqueeze scalar. This came about in #1560. See ONNX graph screenshot down below.
Pull Request Template
Checklist
run-checks all
script has been executed.Related Issues/PRs
Changes
Ignored
new-type to ignore types in the module.Testing