Skip to content

Introduce coloring results#38

Merged
gdalle merged 13 commits intomainfrom
gd/revamp
Aug 8, 2024
Merged

Introduce coloring results#38
gdalle merged 13 commits intomainfrom
gd/revamp

Conversation

@gdalle
Copy link
Copy Markdown
Member

@gdalle gdalle commented Aug 6, 2024

Warning

Breaking change: bumping version to v0.4.0

Source

  • Introduce a new "detailed" interface with three functions: column_coloring_detailed, row_coloring_detailed and symmetric_coloring_detailed. Make the ADTypes interface fall back on those (we'll see once this stabilizes if it is worth upstreaming).
  • Let the detailed coloring functions return an AbstractColoringResult, which must then be used in decompression routines. A minima, this object contains the color vector and the group vector. There is also one specific implementation for SparseMatrixCSC, where decompression indices are stored.
  • Anticipate generalizations of coloring results to 1) bidirectional coloring and 2) indirect recovery with linear systems, thanks to a flexible set of type parameters on AbstractColoringResult{partition,symmetric,decompression}.
  • Merge decompress_columns, decompress_row and decompress_symmetric into a single symbol decompress, using dispatch on the result type to figure out the right algorithm.
  • Relax tests on same sparsity pattern, which added significant overhead.

Docs

  • Update docstrings and public symbols

Tests

  • Adjust to new framework
  • Test in-place decompression too

@amontoison this should be very easy to adapt to sparse GPU matrices

@amontoison
Copy link
Copy Markdown
Collaborator

amontoison commented Aug 6, 2024

Should we add ncolors in SimpleColoringResult by default?
I don't remember if we already have it during the coloring.

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented Aug 6, 2024

You can get it as length(get_groups(coloring_result)). I chose to precompute the groups by default because we frequently need them for differentiation or decompression and it's not very expensive.

Don't mind the CI, I'm currently debugging something that only occurs on LTS

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented Aug 6, 2024

@amontoison any idea how this could ever happen for A::SparseMatrixCSC? It's the reason why CI fails, apparently only on 1.6
image

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented Aug 6, 2024

Okay apparently it's caused by JuliaSparse/SparseArrays.jl#44 and fixed in JuliaLang/julia#40523

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.16%. Comparing base (e8d5f2f) to head (f5098f9).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
+ Coverage   97.81%   99.16%   +1.35%     
==========================================
  Files           9        9              
  Lines         366      360       -6     
==========================================
- Hits          358      357       -1     
+ Misses          8        3       -5     

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

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented Aug 7, 2024

@amontoison I made a few more changes and updated the PR description accordingly, care to take a second look?
Still missing the StarSet handling but otherwise we're nearly good

@amontoison
Copy link
Copy Markdown
Collaborator

LGTM!

@gdalle gdalle merged commit 7fbec68 into main Aug 8, 2024
@gdalle gdalle deleted the gd/revamp branch August 8, 2024 05:54
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.

Coloring results Should decompression be represented as a matrix multiplication?

2 participants