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

Remove dtype rewrite #2528

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Remove dtype rewrite #2528

merged 3 commits into from
Nov 25, 2024

Conversation

ArthurBrussee
Copy link
Contributor

Changes

I did something like

let a = create_tensor(123, u32);
let b = create_tensor(123, a.dtype())

and got weird errors about sizes not being a multiple of 4, which after digging made me find that b was a bool tensor now!

I think that can be a pretty bad footgun, dtype() should return the actual dtype(). Bool tensors might need some reword to properly fix this and not display as u32, but I think that's better than having this API.

@nathanielsimard
Copy link
Member

The display problem should be resolved simply by passing the info that the data is encoding booleans in burn-jit not impacting the burn_tensor apis

@laggui
Copy link
Member

laggui commented Nov 25, 2024

Although less likely to cause issues, this mapping for the bool encoding dtype is also in burn-candle. We should also return U8 for the primitive dtype and not bool in this case.

Since this is specifically for display purposes, perhaps we could fix the Display implementation for tensors? E.g.

writeln!(f, "  shape:  {:?},", self.dims())?;
writeln!(f, "  device:  {:?},", self.device())?;
writeln!(f, "  backend:  {:?},", B::name())?;
writeln!(f, "  kind:  {:?},", K::name())?;

// Bool tensors might be encoded in a different type, which we abstract for the display
let dtype = if TypeId::of::<K::Elem>() == TypeId::of::<bool>() {
    DType::Bool
} else {
    self.primitive.dtype()
};

writeln!(f, "  dtype:  {:?},", dtype.name())?;

@ArthurBrussee ArthurBrussee changed the title Remove dtype rewrite Use bool elem for tensors Nov 25, 2024
@ArthurBrussee ArthurBrussee changed the title Use bool elem for tensors Remove dtype rewrite Nov 25, 2024
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.56%. Comparing base (9c31f75) to head (5d1e443).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
crates/burn-candle/src/tensor.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2528      +/-   ##
==========================================
- Coverage   82.59%   82.56%   -0.04%     
==========================================
  Files         827      827              
  Lines      106712   106855     +143     
==========================================
+ Hits        88143    88226      +83     
- Misses      18569    18629      +60     

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


🚨 Try these New Features:

@laggui laggui merged commit fe3e43a into tracel-ai:main Nov 25, 2024
11 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.

3 participants