Optimized implementation for structured matrices - #139
Draft
gdalle wants to merge 43 commits into
Draft
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
gdalle
marked this pull request as ready for review
October 7, 2024 10:46
gdalle
marked this pull request as draft
October 7, 2024 19:31
6 tasks
Copy link
Copy Markdown
|
Bump on this PR, what else do we need to finalize this PR? I can add the decompression for GPU-based structured matrices. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
If you have some cycles to spare to merge main into this and then add the decompression for banded and block-banded matrices, in a way that works on GPU, as well as tests using JLArrays, that would be nice. Otherwise I was planning to get to it sometime this week, but it would be nice for someone else to get acquainted with the code base. You can take a look at the CUDA extension for inspiration |
Sorry, something went wrong.
Copy link
Copy Markdown
|
Sure thing, I will first start to merge the main branch into this branch. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
Thanks! Ping me if you have any questions |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
Btw, an easier first step would be to fix Diagonal and friends, which probably don't work on GPU either in here |
Sorry, something went wrong.
* Merge main into gd/structured3 * Add the missing docs
Copy link
Copy Markdown
Member
Author
|
@ErikQQY not sure why we still have such a large diff here, and conflicts? |
Sorry, something went wrong.
Copy link
Copy Markdown
Codecov Report❌ Patch coverage is 11.16928% with 509 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #139 +/- ##
=======================================
Coverage ? 26.25%
=======================================
Files ? 22
Lines ? 2236
Branches ? 0
=======================================
Hits ? 587
Misses ? 1649
Partials ? 0 ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
Copy link
Copy Markdown
|
Not sure why it's not working, let me retry |
Sorry, something went wrong.
* 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>
Copy link
Copy Markdown
Member
Author
|
Doesn't seem to have helped? |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
Maybe you're better off starting from scratch instead of fighting against Git? |
Sorry, something went wrong.
Copy link
Copy Markdown
|
So weird I can't merge main into this one 🫠
Do you mean that I should start from a fresh branch based on the current main, and then re-implement the structured coloring and decompression improvements from this PR? |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
Yeah, sometimes it's best to appease the Git gods and start over. You can copy paste anything you need from here |
Sorry, something went wrong.
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.
Fixes #64 and fixes #65 by optimizing structured coloring & decompression for these matrix types:
For BandedMatrix and BlockBandedMatrix, the standard decompression based on SparseMatrixCSC will work fine because their storage is column-major (ref1, ref2). We only need a specific overload for BandedBlockBandedMatrix, which is a BlockMatrix of BandedMatrix-like blocks (so we need to decompress block by block).
General philosophy
Separate coloring from decompression:
Remaning issues
For decompression of BlockBandedMatrices, I still need to figure out JuliaArrays/BlockArrays.jl#418. But it already works because the fallback algorithm relies on the nonzeros of the sparse encoding, so it does not try to access illegal indices outside of the bands.
Implementation details
This is a clean rewrite of some code that was already present in ArrayInterface.jl and FiniteDiff.jl:
https://github.com/JuliaArrays/ArrayInterface.jl/blob/2e2b81bfb53e9e9fc41f26b927fb980afcbfa9f5/src/ArrayInterface.jl#L372-L384
https://github.com/JuliaArrays/ArrayInterface.jl/blob/2e2b81bfb53e9e9fc41f26b927fb980afcbfa9f5/ext/ArrayInterfaceBandedMatricesExt.jl#L68-L72
https://github.com/JuliaArrays/ArrayInterface.jl/blob/2e2b81bfb53e9e9fc41f26b927fb980afcbfa9f5/ext/ArrayInterfaceBlockBandedMatricesExt.jl#L207-L244
https://github.com/JuliaDiff/FiniteDiff.jl/blob/b574440c5d54708aa23a90c48f4a000dd1c6d915/ext/FiniteDiffBandedMatricesExt.jl#L13-L27
I still need to implement optimal decompression for block-banded matrices.
https://github.com/JuliaDiff/FiniteDiff.jl/blob/b574440c5d54708aa23a90c48f4a000dd1c6d915/ext/FiniteDiffBlockBandedMatricesExt.jl#L16-L68