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

feat: expand onnx import #1813

Merged
merged 9 commits into from
May 31, 2024
Merged

Conversation

JachymPutta
Copy link
Contributor

@JachymPutta JachymPutta commented May 26, 2024

Changes

Include the expand operator to burn-import

Testing

Manual
cargo test expand

Related Issues

#1714

@antimora antimora requested review from laggui and antimora May 29, 2024 18:05
@JachymPutta JachymPutta changed the title WIP: expand onnx import feat: expand onnx import May 31, 2024
@JachymPutta JachymPutta marked this pull request as ready for review May 31, 2024 17:36
@JachymPutta JachymPutta force-pushed the jp/expand_onnx_import branch 3 times, most recently from 10cda31 to 95c40fd Compare May 31, 2024 18:17
Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 96.52174% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.43%. Comparing base (0d4374c) to head (c282d08).
Report is 2 commits behind head on main.

Files Patch % Lines
crates/burn-import/src/onnx/dim_inference.rs 91.66% 2 Missing ⚠️
crates/burn-import/src/onnx/op_configuration.rs 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1813      +/-   ##
==========================================
- Coverage   86.44%   86.43%   -0.02%     
==========================================
  Files         759      762       +3     
  Lines       87777    88099     +322     
==========================================
+ Hits        75880    76147     +267     
- Misses      11897    11952      +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@antimora antimora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you for your contribution.

Minor. We should remove a case when we extract shape from Expand node attributes. Spec does not have it compared to other node types. Probably it was left from copying.

Once this is done, I believe we can merge it.

Comment on lines 483 to 484
} else {
node.attrs.get("shape").cloned().map(|v| v.into_i64s())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not applicable for Expand since there is no attribute shape the spec: https://onnx.ai/onnx/operators/onnx__Expand.html. So we should remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right! I will get rid of it:)

Copy link
Collaborator

@antimora antimora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me

Thank you for your contribution!

@antimora antimora merged commit 99e1ba4 into tracel-ai:main May 31, 2024
14 checks passed
LilDojd pushed a commit to LilDojd/burn that referenced this pull request Jun 5, 2024
* feat: added expand to import
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