Skip to content

Fix respectful_similar with SparsityPatternCSC#299

Merged
gdalle merged 5 commits intoJuliaDiff:mainfrom
blegat:similar
Jan 29, 2026
Merged

Fix respectful_similar with SparsityPatternCSC#299
gdalle merged 5 commits intoJuliaDiff:mainfrom
blegat:similar

Conversation

@blegat
Copy link
Copy Markdown
Contributor

@blegat blegat commented Jan 19, 2026

Otherwise, decompress returns a dense matrix when a SparsityPatternCSC is given as argument to coloring, see below:

julia> S = sparse([
           0 0 1 1 0 1
           1 0 0 0 1 0
           0 1 0 0 1 0
           0 1 1 0 0 0
       ])
4×6 SparseMatrixCSC{Int64, Int64} with 9 stored entries:
     1  1    1
 1        1  
   1      1  
   1  1      

julia> P = SparseMatrixColorings.SparsityPatternCSC(S)
4×6 SparseMatrixColorings.SparsityPatternCSC{Int64}:
 false  false   true   true  false   true
  true  false  false  false   true  false
 false   true  false  false   true  false
 false   true   true  false  false  false

julia> problem = ColoringProblem()
ColoringProblem{:nonsymmetric, :column}()

julia> algo = GreedyColoringAlgorithm()
GreedyColoringAlgorithm{:direct, 1, Tuple{NaturalOrder}}((NaturalOrder(),), false)

julia> result = coloring(P, problem, algo); # We give P, not S

julia> B = compress(S, result)
4×3 Matrix{Int64}:
 1  1  1
 1  1  0
 1  1  0
 1  1  0

julia> C = decompress(B, result)
4×6 Matrix{Int64}:
 0  0  1  1  0  1
 1  0  0  0  1  0
 0  1  0  0  1  0
 0  1  1  0  0  0

This affects blegat/ArrayDiff.jl#23 because I just create a SparsityPatternCSC with the detected sparsity pattern and use coloring with this as input.

I can write tests if the change sounds reasonable

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (921c7a0) to head (77d32d5).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #299   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines         2139      2141    +2     
=========================================
+ Hits          2139      2141    +2     

☔ 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.

Copy link
Copy Markdown
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just overload similar instead?

@blegat
Copy link
Copy Markdown
Contributor Author

blegat commented Jan 21, 2026

The issue is that SparsityPatternCSC does not have any field to store the values so it cannot be used for the decompression.
In a sense, coloring only needs the sparsity pattern but decompression needs values so we need some kind of similar_but_with_values ^^ I thought respectful_similar would serve this purpose

@gdalle
Copy link
Copy Markdown
Member

gdalle commented Jan 21, 2026

It does have values since SparsityPatternCSC <: AbstractMatrix{Bool}. And since similar preserves the sparsity pattern for SparseMatrixCSC, I think it would make sense to define

function similar(A::SparsityPatternCSC, ::Type{T}) where {T}
    return SparseArrays.SparseMatrixCSC(
        A.m,
        A.n,
        A.colptr,
        A.rowval,
        similar(A.rowval, T),
    )
end

@blegat
Copy link
Copy Markdown
Contributor Author

blegat commented Jan 21, 2026

Good point, what about similar with Bool ?

@gdalle
Copy link
Copy Markdown
Member

gdalle commented Jan 21, 2026

It should probably also return a SparseMatrixCSC, that way values are modifiable with a surface API

@blegat blegat requested a review from gdalle January 29, 2026 12:42
Copy link
Copy Markdown
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@gdalle gdalle merged commit a7cb154 into JuliaDiff:main Jan 29, 2026
10 checks passed
gdalle added a commit that referenced this pull request Apr 28, 2026
* 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>
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.

3 participants