-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add sparse matrix option with Compressed Sparse Column ordering #693
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #693 +/- ##
==========================================
+ Coverage 93.54% 93.68% +0.13%
==========================================
Files 55 57 +2
Lines 3796 3941 +145
==========================================
+ Hits 3551 3692 +141
- Misses 245 249 +4 ☔ View full report in Codecov by Sentry. |
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 @mattldawson for working on this new feature. Looks good to me.
I could also confirm that the GPU tests pass on Derecho's A100 GPU with that latest nvhpc/24.11 compiler.
class LMatrixPolicy = SparseMatrixPolicy, | ||
class UMatrixPolicy = SparseMatrixPolicy> |
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.
Why do we need this policy for the matrices?
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.
One of the things we wanted to try was using CSC for the L matrix and CSR for the U matrix with the Doolittle algorithm. This let us try that out, but it didn't have a big impact (is this right @sjsprecious ?). But, we're not sure why yet, so I think we wanted to leave the option until we could better understand the performance results.
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.
Matt's right as I originally thought different policies for L/U matrices would make the performance better, but it was actually not. I still think after some optimizations we should see a benefit from this change but I could be wrong. As Matt said, let's leave this option here until we have a better understanding of the performance results.
Adds Compressed Sparse Column ordering option for standard and vector sparse matrix ordering.