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: added slice onnx import #1856

Merged
merged 2 commits into from
Jun 11, 2024
Merged

Conversation

JachymPutta
Copy link
Contributor

@JachymPutta JachymPutta commented Jun 5, 2024

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Helping with #1714

Changes

Added the slice import to onnx

Testing

cargo test -p burn-import --color=always -- --color=always
cargo test -p onnx-tests --color=always

@JachymPutta JachymPutta force-pushed the jp/slice_onnx_import branch from 959c74a to 41c821d Compare June 5, 2024 19:47
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 86.27451% with 21 lines in your changes missing coverage. Please review.

Project coverage is 86.10%. Comparing base (36ed65a) to head (67742c5).
Report is 7 commits behind head on main.

Files Patch % Lines
crates/burn-import/src/onnx/op_configuration.rs 62.22% 17 Missing ⚠️
crates/burn-import/src/onnx/dim_inference.rs 80.95% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1856      +/-   ##
==========================================
- Coverage   86.30%   86.10%   -0.20%     
==========================================
  Files         774      777       +3     
  Lines       90072    90649     +577     
==========================================
+ Hits        77733    78056     +323     
- Misses      12339    12593     +254     

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

@nathanielsimard nathanielsimard requested a review from laggui June 6, 2024 21:04
Copy link
Member

@laggui laggui left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this op! 🙏

A couple of minor requests, and perhaps removing the commented code once everything is completed :)

let input = Tensor::<Backend, 2>::from_floats(
[
[1., 2., 3., 4., 5., 6., 7., 8., 9., 10.],
[1., 2., 3., 4., 5., 6., 7., 8., 9., 10.],
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change the values of the second dim just to make it clear what data is sliced with this test 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😃 right, yeah

Comment on lines 36 to 40
// compine starts and ends into ranges
// example: starts = [0, 0, 0, 0], ends = [1, 1, 1, 1]
// ranges = [0..1, 0..1, 0..1, 0..1]
let starts = &self.starts;
let ends = &self.ends;
Copy link
Member

Choose a reason for hiding this comment

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

I think we still need to check the axes since there is no guarantee that they always including the leading axes (e.g., if axes = [2, 3] for values of starts and ends), but it's not entirely clear even as described in the spec.

If we keep this solution without checking for non leading axes, I would at least parse the values of axes and error on invalid/unsupported values (i.e., skipping leading axes).

crates/burn-import/src/onnx/op_configuration.rs Outdated Show resolved Hide resolved
Copy link
Member

@laggui laggui left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Thanks for making the changes and handling the cases we don't support for now.

@laggui laggui merged commit 671ec8c into tracel-ai:main Jun 11, 2024
14 checks passed
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