Accept AbstractSparseMatrixCSC for decompression

[blegat]

Here is a suggestion for #296 (comment)
I can write tests if the change sounds reasonable

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.52%. Comparing base (0996030) to head (e93a786).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #298      +/-   ##
==========================================
- Coverage   99.39%   95.52%   -3.87%     
==========================================
  Files          20       20              
  Lines        2145     2145              
==========================================
- Hits         2132     2049      -83     
- Misses         13       96      +83     

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

I don't think there are any API specifications on SparseArrays.AbstractSparseMatrixCSC, and since we make heavy use of the fields of SparseMatrixCSC, this seems like a risky move. Can you explain where it comes in handy?

[blegat]

Yes, the decompression relies on a few properties of SparseMatrixCSC. It seems that it needs to be able to

• Get the size
• Get colptr, currently it is accessing it directly via the field but it could call getcolptr instead
• Call nonzeros

In my case, the nonzeros is a view and I want to avoid having to copy it to a Vector so that it fits in a SparseMatrixCSC. So I define a custom AbstractSparseMatrixCSC:
https://github.com/blegat/ArrayDiff.jl/pull/23/files#diff-47c27891e951c8cd946b850dc2df31082624afdf57446c21cb6992f5f4b74aa2R207-R215

The properties that the implementation relies on are that getcolptr is the colptr if a CSC representation, that seems safe to assume and that the nonzeros are sorted in increasing row indices (per column), which is perhaps the one more risky but we may also just consider that it's part of the definition of the CSC format.
The other option would be defining a custom struct in this package (so basically just copy-pasting the one I did in ArrayDiff). Then there is the risk of someone needed yet another struct.
Both options look fine to me, let me know which one you prefer ;)

[gdalle]

The properties that the implementation relies on are that getcolptr is the colptr if a CSC representation, that seems safe to assume and that the nonzeros are sorted in increasing row indices (per column), which is perhaps the one more risky but we may also just consider that it's part of the definition of the CSC format.

There are CSC matrices in the ecosystem where the sorting is not enforced, see eg JuliaGPU/GPUArrays.jl#648. The question is whether there could be subtypes of SparseArrays.AbstractSparseMatrixCSC where that is also the case, and the answer is probably: since there is no documentation, anything goes?

Given that uncertainty, I kinda like the suggestion you made in #296 to split out a decompression subroutine that uses an AbstractVector (probably with 1-based, contiguous indexing checked upfront).

[blegat]

Reading more the implementation of the decompression, it seemed that it was also using the colptr vector while I initially though it was only using the nonzeros when I wrote that comment. Hence the change of mind with this PR. The implementation of the decompression is however not reading the rowvals since it assumes that it is sorted. So one option could be to pass along both the colptr and the nonzeros.
Another alternative is for SMC to define a new function is_rowvals_increasing_per_column with a default implementation returning false, implement it for SparseMatrixCSC and then ArrayDiff could implement it as well for its custom sparse matrix. The, we can check it and error if it returns false. Let me know what you prefer

[gdalle]

I think I'd rather add an internal indirection step like

function decompress!(A::SparseMatrixCSC, ...)
    return decompress_csc!(A.nzval, A.rowval, A.colptr, A.m, A.n, ...)
end

than rely on an incompletely specified AbstractSparseMatrixCSC API. You can then use decompress_csc! in ArrayDiff, until we figure out something cleaner?

[gdalle]

The specific result types we use are not public right now, so this function cannot be public either

[blegat]

It was failing if not included in the docs since it has a docstring.
What do you mean by result types not being public ? decompress! also takes result as argument

[gdalle]

I mean that the precise names and fields of the results which are returned are subject to change.
That's also why I asked whether we wanted to extend decompress_csc! beyond just the acyclic coloring case, because otherwise we have to document that it only works for a specific type of coloring. In any case, I'd rather keep it in the private part of the package and list the docstring among the internals, so that you can start using it but we're free to refine the interface in future versions.

[gdalle]

It seems a bit weird to only add this for the acyclic case, you don't need it for anything else on the JuMP side?

[blegat]

True, I haven't tested with star coloring yet. That's one of the advantage of using SMC, now we can easily test with star coloring as well :)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.52%. Comparing base (f56da29) to head (9469e4d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #298   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files          20       20           
  Lines        2145     2145           
=======================================
  Hits         2049     2049           
  Misses         96       96           

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