Skip to content

Speed up CSC substitution decompression for stars - #354

Open
blegat wants to merge 6 commits into
JuliaDiff:mainfrom
blegat:single-edge-decompression
Open

blegat wants to merge 6 commits into
JuliaDiff:mainfrom
blegat:single-edge-decompression

Conversation

@blegat

@blegat blegat commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Reuse the existing CSC write logic for one-edge trees, whose value can be read directly from the compressed matrix without substitution. Move that logic into a helper shared by the one-edge case and the general substitution path.

Small recovery benchmark for 250 dense 20×20 blocks, run with SMC before and after this change:

using BenchmarkTools, LinearAlgebra, SparseArrays, Statistics
import SparseMatrixColorings as SMC

A = kron(spdiagm(0 => ones(250)), sparse(ones(20, 20)))
result = SMC.coloring(
    SMC.SparsityPatternCSC(A),
    SMC.ColoringProblem(; structure=:symmetric),
    SMC.GreedyColoringAlgorithm(; decompression=:substitution),
)
B = Matrix(SMC.compress(A, result))
L = tril(A)
values, colptr = similar(nonzeros(L)), L.colptr
trial = @benchmark SMC.decompress_csc!($values, $colptr, $B, $result, :L) evals=1
@assert values ≈ nonzeros(L)
display(median(trial))

Measured medians for the same block pattern (5,000 variables); full Hessian evaluation was benchmarked separately through MOI:

Implementation Recovery only Full Hessian
Legacy MOI 98.98 μs 521.63 μs
SMC before 252.67 μs 662.93 μs
SMC after 143.89 μs 527.16 μs

On Julia 1.13.1 with Intel i7-12700K.

@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 1.81818% with 108 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.40%. Comparing base (7d894ca) to head (9bb0e21).

Files with missing lines Patch % Lines
src/decompression.jl 0.00% 108 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
- Coverage   46.91%   43.40%   -3.51%     
==========================================
  Files          20       20              
  Lines        2498     2534      +36     
==========================================
- Hits         1172     1100      -72     
- Misses       1326     1434     +108     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/decompression.jl Outdated
@blegat
blegat force-pushed the single-edge-decompression branch from f6fc483 to 016e0b0 Compare October 21, 2026 15:35
@amontoison

Copy link
Copy Markdown
Collaborator

@blegat Can you do the same modification for the acyclic bicoloring?
The title of the PR is also wrong now.

Comment thread src/decompression.jl Outdated
@blegat blegat changed the title Speed up CSC substitution decompression for one-edge trees Speed up CSC substitution decompression for stars Oct 22, 2026
@amontoison

amontoison commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

@blegat You also forgot to update the method decompress!(A::AbstractMatrix, B, ::TreeSetColoringResult, uplo). I can merge the PR after that.

@amontoison

Copy link
Copy Markdown
Collaborator

@blegat
We have a typo in the docstring of is_star (src/coloring.jl, struct TreeSet): "three" should be "tree".

"For a tree index `1 <= k <= nt`, `is_star[k]` indicates whether the `k`th tree is a star."

I also want to fix a comment in the TreeSet constructor (src/coloring.jl, where first_vertex and last_vertex are computed).
This one predates the PR, but while we are here: "so we add to add an offset" is a typo, and the comment doesn't say why the offset is needed.
I suggest:

# Note: tree_edge_indices contains the positions of the first and last edges.
# Each tree has one more vertex than edges, so the k-1 previous trees shift
# the vertex positions by k-1 with respect to the edge positions.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants