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

[burn-import] Add other DataType by conversion to i32 #915

Conversation

QuantumEntangledAndy
Copy link

Pull Request Template

Checklist

  • Confirm that run-checks script has been executed.

Related Issues/PRs

  • None

Changes

The current burn-import main dosent' implement any other data types than int32.

Instead it marks it as TODO with a note abour converting to i32

https://github.com/burn-rs/burn/blob/96524d40a1cffc8c13394f70775ce53d5a1abd3b/burn-import/src/onnx/proto_conversion.rs#L37-L40

This fills out that todo by converting to i32.

Testing

  • Not yet, just discussing the changes first to see what is the scope of the PR is and where other changes should be made

Should I also for example make any adjustments to the cast nodes

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (96524d4) 86.49% compared to head (50ee0df) 86.41%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #915      +/-   ##
==========================================
- Coverage   86.49%   86.41%   -0.08%     
==========================================
  Files         471      471              
  Lines       44217    44256      +39     
==========================================
  Hits        38244    38244              
- Misses       5973     6012      +39     
Files Coverage Δ
burn-import/src/onnx/proto_conversion.rs 50.25% <0.00%> (-12.57%) ⬇️

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

@antimora
Copy link
Collaborator

I was traveling and I'll comment on this in coming days.

@antimora
Copy link
Collaborator

It looks good so far. Let me know if you want to continue working on this. Sorry this PR dropped from my radar since I was traveling and I was working on the tasks.

@QuantumEntangledAndy
Copy link
Author

I'm involved in some other major projects at the moment so don't have time to really work it up right now.

Also worried about the larger implications of converting to 32. Usually a model uses 8bit to reduce memory foot prints. Ideally it would be nice to do u8 etc properly but I think that would mean changing all the back ends too.

@antimora
Copy link
Collaborator

Ok. Let's close it for now. It would be easier if we have a real use case example with an onnx. Otherwise we will be working abstract.

Feel free to reopen if you decide to work on it.

@antimora antimora closed this Jan 30, 2024
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