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.20% 99.30% +0.09%
==========================================
Files 21 21
Lines 2150 2169 +19
==========================================
+ Hits 2133 2154 +21
+ Misses 17 15 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I propose to merge first the support of AMD GPUs (#274). |
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.