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

Convert reduce_dim_naive kernel to use the #[cube] derive macro #2117

Merged
merged 15 commits into from
Aug 14, 2024

Conversation

cBournhonesque
Copy link
Contributor

@cBournhonesque cBournhonesque commented Aug 6, 2024

Pull Request Template

Following a discussion with @nathanielsimard on discord, I wanted to try to convert the reduce kernel to use the cube derive macro.
(originally because I wanted to know how to set the CubeCount for a kernel).

The new macro is pretty ergonomic, it's actually crazy that it also works for impl Trait.
I have a couple remaining issues though:

Checklist

  • Implement CubeType for a tuple of CubeTypes: Implement CubeType for a tuple of CubeTypes cubecl#48
  • How do I get the max value/min value of a Numeric? Do I need to add a trait for it?
  • Need to add the computation of the CubeCount
  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Testing

Unit tests

@cBournhonesque cBournhonesque marked this pull request as ready for review August 12, 2024 21:24
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.

Some changes to be done, but that's crazy how flexible cube is, I never though we would be able to migrate the reduce trait to cube with that amount of ease.

crates/burn-jit/src/kernel/reduce/naive/base.rs Outdated Show resolved Hide resolved
@cBournhonesque
Copy link
Contributor Author

cBournhonesque commented Aug 13, 2024

Some changes to be done, but that's crazy how flexible cube is, I never though we would be able to migrate the reduce trait to cube with that amount of ease.

Yes, it's crazy that the code just looks like normal rust but is still handled correctly! Very nice that it works across traits as well

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 21.42857% with 77 lines in your changes missing coverage. Please review.

Project coverage is 86.08%. Comparing base (ff8d030) to head (0e5d280).

Files Patch % Lines
crates/burn-jit/src/kernel/reduce/naive/shader.rs 45.71% 19 Missing ⚠️
crates/burn-jit/src/kernel/reduce/naive/argmax.rs 5.26% 18 Missing ⚠️
crates/burn-jit/src/kernel/reduce/naive/argmin.rs 5.88% 16 Missing ⚠️
...rates/burn-jit/src/kernel/reduce/naive/prod_dim.rs 10.00% 9 Missing ⚠️
crates/burn-jit/src/kernel/reduce/naive/sum_dim.rs 10.00% 9 Missing ⚠️
...rates/burn-jit/src/kernel/reduce/naive/mean_dim.rs 14.28% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2117      +/-   ##
==========================================
- Coverage   86.21%   86.08%   -0.13%     
==========================================
  Files         694      694              
  Lines       88854    88747     -107     
==========================================
- Hits        76606    76399     -207     
- Misses      12248    12348     +100     

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

@nathanielsimard nathanielsimard merged commit 0435721 into tracel-ai:main Aug 14, 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.

3 participants