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

Implement Element for bool #1878

Merged
merged 10 commits into from
Jun 14, 2024
Merged

Implement Element for bool #1878

merged 10 commits into from
Jun 14, 2024

Conversation

laggui
Copy link
Member

@laggui laggui commented Jun 11, 2024

Checklist

  • Confirmed that run-checks all script has been executed.

Related Issues/PRs

Required for #1535 and Data refactor

Changes

Since the traits Zero, One and ToPrimitive from num-traits could not be implemented for bool:

  • Added new Zero and One element identities (without Add and Mul requirement) Removed the traits (previously kept for historical reasons)
  • Added new ToElement trait (adapted from num_traits::ToPrimitive)

Element is now implemented for bool, which should simplify a couple of things.

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 47.34982% with 149 lines in your changes missing coverage. Please review.

Project coverage is 86.02%. Comparing base (5e58ae1) to head (095199d).

Files Patch % Lines
crates/burn-tensor/src/tensor/element/cast.rs 43.85% 137 Missing ⚠️
crates/burn-tensor/src/tensor/element/base.rs 47.05% 9 Missing ⚠️
crates/burn-ndarray/src/ops/macros.rs 0.00% 1 Missing ⚠️
crates/burn-tensor/src/tensor/ops/int_tensor.rs 0.00% 1 Missing ⚠️
crates/burn-tensor/src/tensor/ops/tensor.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1878      +/-   ##
==========================================
- Coverage   86.14%   86.02%   -0.12%     
==========================================
  Files         778      779       +1     
  Lines       90827    91064     +237     
==========================================
+ Hits        78245    78342      +97     
- Misses      12582    12722     +140     

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

@laggui
Copy link
Member Author

laggui commented Jun 13, 2024

@nathanielsimard Removed One and Zero identities as discussed.

@laggui
Copy link
Member Author

laggui commented Jun 13, 2024

Not sure why clippy failed on that commit specifically, seems unrelated.

/edit: just saw PR #1886 fixes that

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.

LGTM

@antimora
Copy link
Collaborator

Will this fix #1535?

@laggui
Copy link
Member Author

laggui commented Jun 14, 2024

Will this fix #1535?

The ops won't be implemented in this PR but with these changes they could be refactored properly.

Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

LGTM, however I would add num_trait to the NOTICE.md file before merging.

@laggui laggui merged commit 5252440 into main Jun 14, 2024
12 checks passed
@laggui laggui deleted the fix/element/bool branch June 14, 2024 13:02
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.

None yet

3 participants