Add an option decompression_uplo for symmetric results - #294
Open
amontoison wants to merge 1 commit into
Open
amontoison wants to merge 1 commit into
amontoison wants to merge 1 commit into
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:
|
Contributor
Benchmark Results
|
Member
|
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 |
Collaborator
Author
|
Yes, it should make the The one in the result always takes precedence. |
Member
|
I took a second look and the semantics of this are a bit confusing to me. From what I understand:
|
Collaborator
Author
You are right!
Yes, because the option |
gdalle
requested changes
Feb 27, 2026
| nzA = nonzeros(A) | ||
| check_compatible_pattern(A, ag, uplo) | ||
| if uplo == :F | ||
| if result.decompression_uplo == uplo |
Member
There was a problem hiding this comment.
Why is this case not considered for the TreeSetColoringResult below?
Collaborator
Author
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
end
amontoison
force-pushed
the
am/decompression_uplo
branch
from
September 7, 2026 20:37
a55cce6 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.17% 99.18%
=======================================
Files 22 22
Lines 2304 2325 +21
=======================================
+ Hits 2285 2306 +21
Misses 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
I propose to merge first the support of AMD GPUs (#274). |
amontoison
force-pushed
the
am/decompression_uplo
branch
from
September 9, 2026 03:58
c72b0c8 to
dd445bd
Compare
amontoison
added a commit
that referenced
this pull request
Sep 19, 2026
`ColoringProblem` gains a third type parameter `uplo`, so the triangle to recover is chosen once, up front, and propagates all the way to decompression. This answers the review question on #294: the trailing `uplo` argument of `decompress!` / `decompress_single_color!` is gone, so a result and a call site can no longer disagree about which triangle is meant. - `ColoringProblem{structure,partition,uplo}`, with `uplo` defaulting to `:F`. `ColoringProblem{structure,partition}()` still works, via an outer constructor on the partially applied UnionAll. - `uplo != :F` is rejected for non-symmetric problems: `ColumnColoringResult` and `RowColoringResult` have no triangle logic, so the option would be silently dropped. The `:L` used internally by bicoloring is unaffected, it is passed explicitly to the inner symmetric result. - `compressed_indices` only holds the requested triangle, so the star-coloring decompression kernels now walk it with a running counter instead of indexing it by position in the full pattern. In the single-color triangle fallback the hub is not necessarily the current column, so the spoke is decoded from the stored value rather than assumed. - Out-of-place `decompress` allocates a target with the right triangular pattern for symmetric results. - GPU extensions reject `uplo != :F` at coloring time (a real error rather than an `@assert`, since it is now user-reachable) instead of at decompression. Breaking, hence 0.4.28 -> 0.5.0. Migration: `decompress!(A, B, result, :U)` becomes `result_U = coloring(S, ColoringProblem(; structure=:symmetric, uplo=:U), algo)` followed by `decompress!(A, B, result_U)`. One result now yields exactly one triangle, so build one result per triangle if you need several. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
amontoison
force-pushed
the
am/decompression_uplo
branch
from
September 20, 2026 04:08
28cbae0 to
026b715
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.