-
Notifications
You must be signed in to change notification settings - Fork 474
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
Add shape ONNX op support #1639
Conversation
Didn't realize torch always exports the shape op with a cast.. that's causing the CI to fail. PR #1634 will fix that, but will fix the included graph for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1639 +/- ##
==========================================
+ Coverage 86.30% 86.34% +0.04%
==========================================
Files 692 691 -1
Lines 79998 79523 -475
==========================================
- Hits 69040 68666 -374
+ Misses 10958 10857 -101 ☔ View full report in Codecov by Sentry. |
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.
One left over comment change. Approving ahead.
We might have to revisit the output type. I know in the standard arrays are treated as 1d tensors but it would worth if we use shape output type because when we will have to feed into a reshape operation, it would be convenient to receive in rust array type.
For now, if your solution works for your use case, we should use it till we encounter other use cases, such as reshape. With ONNX, I am taking a conservative approach because ONNX spec is too complex to implement at once.
pub fn shape_config(curr: &Node) -> (usize, usize) { | ||
if curr.inputs.len() != 1 { | ||
panic!( | ||
"Flatten: multiple inputs are not supported (got {:?})", |
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.
Flatten => Shape
I initially thought about returning a shape array but then when I started testing I noticed that most of the time in an ONNX model the shape for a dimension is retrieved with But I agree, importing ONNX is not straightforward. |
Related Issues/PRs
Progress towards #1544
Changes
Added shape ONNX op import support
Testing
Added unit tests for codegen and expected outputs