Document fields of TreeSet by blegat · Pull Request #296 · JuliaDiff/SparseMatrixColorings.jl

blegat · 2026-01-09T12:43:47Z

I needed to understand these from the implementation for the purpose of blegat/ArrayDiff.jl#19 so I thought I might as well document them now that I understand what they represent :)

gdalle · 2026-01-09T13:15:34Z

Note that these are internal structs which were never meant for outside consumption. If you need them to be public, can you give some more detail as to why?

gdalle · 2026-01-09T14:32:12Z

To clarify, more documentation, even for private objects, is always a good thing. I just wanna know if we can also make the public-facing API sufficient for your uses.

blegat · 2026-01-09T15:32:53Z

In JuMP, we compute the sparsity pattern of the Hessian of the objective I0, J0 and of each constraint Ik, Jk.
We then give the solver the concatenation vcat(I0, I1, ..., Im) and vcat(J0, J1, ..., Jm).
The, the solver give use a vector V which has the same length as the concatenation of the indices and we just need to fill it.
It seems the interface of SMC requires that we give a SparseMatrixCSC for the decompression:
https://github.com/gdalle/SparseMatrixColorings.jl/blob/31999f1409f5398ccd6224fb76f8dccdfe2d50c6/src/decompression.jl#L589
Instead, I would like for the objective and each constraint to just take a view of the corresponding part of V, and give it to SMC.
We can probably split this decompress! in two, one part that takes the nonzeros out of the SparseMatrixCSC and then redirects to another decompress! that just takes an AbstractVector of nonzeros and we we could use that second one from ArrayDiff

gdalle · 2026-01-09T18:07:46Z

Yeah that sounds feasible, but I'm more worried about the view part. Why do you need a view of V, and what kind of view?

amontoison · 2026-01-09T19:24:41Z

It needs a continuous view because JuMP is doing a coloring for the Hessian of the objective and then for the Hessian of each constraints.
They are not doing like ADNLPModels where we do directly a coloring of the Hessian of the Lagrangian.
After the computation of the nonzeros of each compressed Hessian, they need to do the decompression in the related part of the vector V to recover the sparse Hessian.

@blegat I am right?

amontoison · 2026-01-09T19:28:51Z

+    """
+    contains the reverse breadth first (BFS) traversal order for each tree in the forest.
+    More precisely, given an edge `(u, v)` of index `i`,
+    `reverse_bfs_order[i]` is either `(u, v)` or `(v, u)`.


It is not right, i is not an edge index.

Should be fixed now

amontoison · 2026-01-09T19:33:45Z

    # Number of edges treated
    num_edges_treated = zero(T)

-    # reverse_bfs_orders contains the reverse breadth first (BFS) traversal order for each tree in the forest


We should keep this comment.
What do think of:

# reverse_bfs_orders will contain the reverse breadth first (BFS) traversal order for each tree in the forest concatenated

I simply moved it into the comment of the field of the struct. I feel that the definition of what reverse_bfs_orders will be should go there and here we should comment on why what we do achieve this

I added a new comment clarifying the confusion with the root of the Forest union-find datastructure that we thought we be useful during our discussion on the phone

amontoison · 2026-01-09T19:38:34Z

    is_star::Vector{Bool}
+    "`tree_edge_indices[1]` is one and `tree_edge_indices[k+1] - tree_edge_indices[k]` is the number of edges in the `k`th tree"
    tree_edge_indices::Vector{T}
+    "numbers of 2-colored trees for which trees sharing the same 2 colors have disjoint vertices"


I don't think they have disjoint vertices. When the matrix is dense, we have n(n+1)/2 trees with two vertices (one for each edge) but only n vertices.

If a red vertex u is adjacent to both a blue and yellow vertex then the red-blue tree and red-yellow tree have the u vertex in common so they don't have disjoint vertices

Ahah, I contradicted myself in my reply. I meant disjoint edges indeed

Fixed it in the last commit

Co-authored-by: Alexis Montoison <35051714+amontoison@users.noreply.github.com>

codecov · 2026-01-10T08:44:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (31999f1) to head (20ab117).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##              main      #296    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           20        20            
  Lines         2027      2139   +112     
==========================================
+ Hits          2027      2139   +112

☔ View full report in Codecov by Sentry.
📢 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.

@gdalle

* Bump julia-actions/julia-downgrade-compat from 1 to 2 (#258) Bumps [julia-actions/julia-downgrade-compat](https://github.com/julia-actions/julia-downgrade-compat) from 1 to 2. - [Release notes](https://github.com/julia-actions/julia-downgrade-compat/releases) - [Commits](julia-actions/julia-downgrade-compat@v1...v2) --- updated-dependencies: - dependency-name: julia-actions/julia-downgrade-compat dependency-version: '2' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump actions/checkout from 4 to 5 (#259) * ensure `UnsupportedDecompressionError <: Exception` (#260) Not sure if there is any benefit, but I suppose the intention is for this type, as an exception type, to subtype `Exception`. * Bump peter-evans/create-or-update-comment from 4 to 5 (#261) * Bump peter-evans/find-comment from 3 to 4 (#262) Bumps [peter-evans/find-comment](https://github.com/peter-evans/find-comment) from 3 to 4. - [Release notes](https://github.com/peter-evans/find-comment/releases) - [Commits](peter-evans/find-comment@v3...v4) --- updated-dependencies: - dependency-name: peter-evans/find-comment dependency-version: '4' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Allow picking the best of several orders for GreedyColoringAlgorithm (#265) * Allow picking the best of several orders for GreedyColoringAlgorithm * Fix type params * Tuple * Better tests * Better test * Fix foc * Fix type inference inside closure * Fix seed * Test on 1.11 * Avoid duplicate remap_colors * chore: bump version to 0.4.22 (#268) * chore: fix typo in package UUID (#269) * Make any coloring algorithm compatible with SMC (#263) * Make any coloring algorithm compatible with SMC * Fix StackOverflow * Run tests on 1.11 * Start working on optimal coloring * Fix tests * Improve feasibility check * Format * Fix * Fix * Typo * Use HiGHS * Remove OptimalColoringAlgorithm * Remove test deps * Don't handle forced colors in acyclic * Run GPU CI on 1.11 too * Actually use forced colors * Format * Deactivate JuliaFormatter (can't figure out why it fails) * Optimal coloring algorithm with JuMP formulation (#271) * Add a file postprocessing.jl (#275) * Rename has_diagonal into augmented_graph (#273) * Rename has_diagonal into augmented_graph * Fix a typo in postprocessing.jl * Update src/graph.jl Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> --------- Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * Try tests on Julia 1.12 (#276) * Bump SMC to v0.4.23 (#277) * Patch failing alloc test (#278) * Add postprocess_with_star_set! and postprocess_with_tree_set! (#279) * Bump actions/checkout from 5 to 6 (#282) Bumps [actions/checkout](https://github.com/actions/checkout) from 5 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v5...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Replace is_solved_and_feasible with assert_is_solved_and_feasible (#284) * Fix JET tests (#285) * Fix JET tests * Move JuliaFormatter to action * Fix formatting * Use diagonal_indices in the general decompress! for acyclic coloring (#287) * Enhance decompress! for bicoloring (#288) * Add a compat entry for CUDA.jl in test/Project.toml (#293) * Add nb_self_loops in AdjacencyGraph (#290) * Remove unused arguments from internal functions (#295) * Add test functions substitutable_columns and substitutable_bidirectional (#297) * Document fields of TreeSet (#296) * Fix respectful_similar with SparsityPatternCSC (#299) * Fix respectful_similar with SparsityPatternCSC * Add test * Fix format * Add test --------- Co-authored-by: Alexis Montoison <35051714+amontoison@users.noreply.github.com> * Fix coloring with empty matrix as input (#300) * Fix coloring with empty matrix as input * Update src/result.jl Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * Add check * Fix format * Change check * Apply suggestion from @gdalle --------- Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * test: skip failing MiniZinc tests (#305) * Fix eltype invalidation for SparsityPatternCSC (#304) SparsityPatternCSC{Ti} <: AbstractMatrix{Bool} but the custom Base.eltype method was returning Ti (the index type) instead of Bool. This caused 24,906 method invalidations when loading the package, as it invalidated the backedge from Base.eltype(::AbstractArray). Change the custom method to SparseArrays.indtype instead, since that's what the type parameter Ti actually represents. The eltype is now correctly inherited from AbstractMatrix{Bool}. Co-authored-by: ChrisRackauckas-Claude <accounts@chrisrackauckas.com> Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * chore: bump version (#306) * Bump julia-actions/cache from 2 to 3 (#307) * Update repo owner to JuliaDiff org (#309) * Update repo owner * Bump version * Switch GPU CI to buildkite * Add badge * Fix buildkite badge [skip tests] (#310) * Accept AbstractSparseMatrixCSC for decompression (#298) * Accept AbstractSparseMatrixCSC for decompression * Define decompress_csc * Remove unused import of AbstractSparseMatrixCSC * Apply suggestion from @blegat * Change return statement to return nothing * Apply suggestion from @blegat * Add to docs * Apply suggestion from @gdalle Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * Move to internals --------- Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * Bump version (#311) * Bump codecov/codecov-action from 5 to 6 (#312) Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5 to 6. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v5...v6) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: bump CUDA compat to v6 (cuSPARSE now a separate package) (#314) * chore: bump CUDA compat to v6 (cuSPARSE now a separate package) * More renaming * Format * Fix compression? * Generic compression and result * Import * No generic result * Fix ambiguity * Bump julia-actions/setup-julia from 2 to 3 (#315) Bumps [julia-actions/setup-julia](https://github.com/julia-actions/setup-julia) from 2 to 3. - [Release notes](https://github.com/julia-actions/setup-julia/releases) - [Commits](julia-actions/setup-julia@v2...v3) --- updated-dependencies: - dependency-name: julia-actions/setup-julia dependency-version: '3' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Neven Sajko <4944410+nsajko@users.noreply.github.com> Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> Co-authored-by: Alexis Montoison <35051714+amontoison@users.noreply.github.com> Co-authored-by: Benoît Legat <benoit.legat@gmail.com> Co-authored-by: Chris Rackauckas - Beep Boop Edition <admin@chrisrackauckas.com> Co-authored-by: ChrisRackauckas-Claude <accounts@chrisrackauckas.com>

Document fields of TreeSet

0ce6e42

amontoison reviewed Jan 9, 2026

View reviewed changes

Comment thread src/coloring.jl Outdated

amontoison reviewed Jan 9, 2026

View reviewed changes

Comment thread src/coloring.jl Outdated

amontoison reviewed Jan 9, 2026

View reviewed changes

amontoison added the documentation Improvements or additions to documentation label Jan 9, 2026

Update src/coloring.jl

8b377be

Co-authored-by: Alexis Montoison <35051714+amontoison@users.noreply.github.com>

blegat commented Jan 10, 2026

View reviewed changes

Comment thread src/coloring.jl Outdated

blegat added 3 commits January 10, 2026 09:54

Update src/coloring.jl

d25581b

Address comments

4cd18b5

Fix

20ab117

blegat mentioned this pull request Jan 19, 2026

Accept AbstractSparseMatrixCSC for decompression #298

Merged

blegat requested a review from amontoison January 29, 2026 08:23

amontoison reviewed Jan 29, 2026

View reviewed changes

Comment thread src/coloring.jl

amontoison approved these changes Jan 29, 2026

View reviewed changes

amontoison merged commit 921c7a0 into JuliaDiff:main Jan 29, 2026
10 checks passed

Conversation

blegat commented Jan 9, 2026

Uh oh!

gdalle commented Jan 9, 2026

Uh oh!

gdalle commented Jan 9, 2026

Uh oh!

blegat commented Jan 9, 2026 • edited by amontoison Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdalle commented Jan 9, 2026

Uh oh!

amontoison commented Jan 9, 2026

Uh oh!

amontoison Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

blegat Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amontoison Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blegat Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

blegat Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amontoison Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

blegat Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

blegat Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

blegat Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

blegat commented Jan 9, 2026 •

edited by amontoison

Loading

amontoison Jan 9, 2026 •

edited

Loading

blegat Jan 19, 2026 •

edited

Loading

codecov Bot commented Jan 10, 2026 •

edited

Loading