Add an option decompression_uplo for symmetric results - #294
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 2027 2045 +18
=========================================
+ Hits 2027 2045 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark Results
|
|
I'm not sure I understand what this PR does. If we pre-specify the type of decompression when we do the coloring, doesn't it make the |
|
Yes, it should make the The one in the result always takes precedence. |
|
I took a second look and the semantics of this are a bit confusing to me. From what I understand:
|
You are right!
Yes, because the option |
| nzA = nonzeros(A) | ||
| check_compatible_pattern(A, ag, uplo) | ||
| if uplo == :F | ||
| if result.decompression_uplo == uplo |
There was a problem hiding this comment.
Why is this case not considered for the TreeSetColoringResult below?
There was a problem hiding this comment.
Because we only gain on the storage when the decompression is by substitution.
The code for the decompression itself is not impacted.
However, I forgot to add an assertion.
I fixed it in my last commit such that:
function decompress!(
A::SparseMatrixCSC{R},
B::AbstractMatrix{R},
result::TreeSetColoringResult,
uplo::Symbol=:F,
) where {R<:Real}
check_compatible_pattern(A, result.ag, uplo)
@assert result.decompression_uplo == uplo || result.decompression_uplo == :F
decompress_csc!(nonzeros(A), A.colptr, B, result, uplo)
return A
enda55cce6 to
c72b0c8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
+ Coverage 99.00% 99.18% +0.18%
==========================================
Files 22 22
Lines 2303 2324 +21
==========================================
+ Hits 2280 2305 +25
+ Misses 23 19 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I propose to merge first the support of AMD GPUs (#274). |
c72b0c8 to
dd445bd
Compare
Add an argument
decompression_uploforStarSetColoringResultandTreeSetColoringResultsuch that we can specialize the decompression for bicoloring.We can always use
decompression_uplo = :Lfor the bicoloring.For the bicoloring,
decompress!only use the specialized version forSparseMatrixCSCsince #288.Everything is internal so it is not breaking.
Replace #289.
I will wait a review of Guillaume before that I merge this PR and future modifications.