-
Notifications
You must be signed in to change notification settings - Fork 54
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
Distributed Compressed Sparse Column Matrix #1377
Conversation
I just discovered something that I should have seen a long time ago.
It is still possible for us to implement |
Thank you for the PR! |
1 similar comment
Thank you for the PR! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1377 +/- ##
==========================================
- Coverage 91.76% 91.74% -0.03%
==========================================
Files 80 80
Lines 11640 11683 +43
==========================================
+ Hits 10682 10719 +37
- Misses 958 964 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@Mystic-Slice thank you so much for this and apologies for the delay. It's OK not to provide the element-wise operations for CSC if torch doesn't implement them yet. The basic operation we want to be able to provide with this class, is matrix multiplication CSR @ CSC, when Is this PR, the CSC class, ready for review @Mystic-Slice ? Thanks again, we highly appreciate your contribution! |
Thank you for the PR! |
Hi @ClaudiaComito If tests aren't a blocker for review, we can proceed with that. |
Thank you for the PR! |
Hi @Mystic-Slice, thanks, the tests make the review so much easier, so please go ahead and sketch the tests first. Thanks a lot! |
Thank you for the PR! |
@Mystic-Slice on the CUDA runner, with PyTorch 2.2, we get the following error:
Many test files return the same error. But we can't reproduce it with other PRs. Can you check it out? |
|
Thank you for the PR! |
@Mystic-Slice this looks amazing. Will you add your name to the CITATION.cff file, under the |
Done! |
Thank you for the PR! |
Thank you for the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work @Mystic-Slice ! 👏🏼
@Mystic-Slice one single test fails with torch<2, see below. From my side it's absolutely OK to skip the test when torch<2
|
@ClaudiaComito |
Thank you for the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Mystic-Slice ! (take 2)
Seems like it didnt work |
Thank you for the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take 3!
Due Diligence
Description
A new format similar to the one implemented in #1028.
Most code from the previous implementation is common for both formats.
Steps:
NOTE: In the future, when arithmetic operations are supported by
torch
forsparse_csc
, they can be enabled forDCSC_matrix
by restoring the code removed in commit 6d727af. The binary operator has already been modified to be generic for bothDCSR_matrix
andDCSC_matrix
.Type of change
New feature