-
Notifications
You must be signed in to change notification settings - Fork 46
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
Fix dimensions for 0D scalars #476
Conversation
Thanks for spotting / correcting that. |
@fdwr noticed while checking the PR queue this needs a rebase. Good discussion in this PR should be factored in too I believe. |
Other things that should be rolled into this PR:
|
Build completes now. If I should remove status text containing MLCommandEncoder entirely instead, let me know. |
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!
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.
Thanks, looks good!
SHA: c320472 Reason: push, by fdwr Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
From issue: #390
A 0D tensor shape vs a 1D tensor with a single element are semantically and functionally distinct, but the current spec mistreats 0D scalars and 1D tensors like they are the same thing, and this messes with the shape inference for operators like
reduceSum
(with all axes andkeepDimensions = false
) orgather
where the output rank depends on the correct rank for input and indices tensors. This caused models like Segment Anything and Stable Diffusion to fail while prototyping, and the issue arose again during Chromium and ORT WebNN EP code review.[1,2,3]
- 3D tensor shape[1,2]
- 2D tensor shape[1]
- 1D tensor shape[]
- 0D tensor shape (scalar) <--- (every known ML library represents 0D scalars via the shape[]
)Fixing this in the Chromium fork needed changing just two lines, and we want to address this sooner than later before changing just a few lines becomes multiple lines.
Preview | Diff