SparseArrays.jl icon indicating copy to clipboard operation
SparseArrays.jl copied to clipboard

Export AbstractSparseMatrixCSC + interface

Open j-fu opened this issue 2 years ago • 13 comments

See discussion in https://github.com/JuliaSparse/SparseArrays.jl/issues/265

j-fu avatar Jun 01 '23 14:06 j-fu

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.24%. Comparing base (81d49e9) to head (4fc922a).

Files Patch % Lines
src/sparsematrix.jl 50.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
+ Coverage   84.11%   84.24%   +0.13%     
==========================================
  Files          12       13       +1     
  Lines        9102     9129      +27     
==========================================
+ Hits         7656     7691      +35     
+ Misses       1446     1438       -8     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 12 '23 17:07 codecov[bot]

Similar question for sparse(m::AbstractSparseMatrixCSC). In 1.10 it returns a copy(m). Which may be reasonable. One could argue as well (I tend to prefer this) that the return value should be a SparseMatrixCSC like with the other methods of sparse.

j-fu avatar May 29 '24 14:05 j-fu

Generally, the thought with sparse has been that it shouldn't be seen as SparseMatrixCSC specific.

ViralBShah avatar May 29 '24 15:05 ViralBShah

My concern with exporting was that wouldn't we better off with something more general? AbstractSparseMatrixCSC feels too specialized a thing as an abstract type. But of course, it isn't a huge change per se.

@rayegun Any thoughts?

ViralBShah avatar May 29 '24 20:05 ViralBShah

Pinging @KristofferC and @fredrikekre on any thoughts here. Another set of eyes would be good here. While I don't see a major issue, a public API would be a big commitment going forward, and AbstractSparseMatrixCSC feels like a bit of a specialized abstract class.

ViralBShah avatar Aug 18 '24 13:08 ViralBShah

... I'd like to come back to this - Something more general would of course be appreciated, as e.g. discussed in #538 . OTOH, as a matter of fact the interface is used in LinearSolve.jl, see e.g. here and here. So IMHO it would be good to have this fixed as public API. May be instead of exporting it could be made public now.

j-fu avatar Aug 18 '24 13:08 j-fu

@ChrisRackauckas, @oscardssmith, what do you think about this ?

j-fu avatar Aug 20 '24 13:08 j-fu

I think we need to write down what that expected interface is if you're going to consider it public. Like https://docs.julialang.org/en/v1/manual/interfaces/ . Can you try doing a first draft of it?

Maybe @mbauman and @timholy would be good to have on-board as well since they drafted the original array interface and I know they have comments on what a sparse array interface would need to have.

ChrisRackauckas avatar Aug 20 '24 13:08 ChrisRackauckas

I started a discussion in #559. Not sure if this is the right place. See also #538.

j-fu avatar Aug 20 '24 20:08 j-fu

Do Diagonal, Bidiagonal, Tridiagonal, and SymTridiagonal all support this interface? They're examples of sparse matrices even if they are not subtypes of AbstractSparseMatrix. Would a CSR matrix be expected to support getcolptr? What would that even mean?

The deepest thinking I've done on a real interface for sparse arrays of all kinds is in https://github.com/timholy/ArrayIteration.jl (unfinished and effectively abandoned for lack of time). That interface is not so tightly coupled to a particular internal representation. A good start is to ask yourself the question, "what is needed to write algorithms on sparse matrices even if I don't know how they are stored?"

timholy avatar Aug 20 '24 22:08 timholy

... Certainly, as CSR matrix should have getrowptr and not getcolptr... Very interesting discussion in ArrayIterators.jl !

I see two strands of discussion here (as I tried to outline in #559):

  • "what is needed to write algorithms on sparse matrices even if I don't know how they are stored?" -- which needs time (which I might not have as well...) and dedication
  • Low level interfaces for coupling to all kinds of (often binary) packages which rely upon the concrete CSC or CSR structure. In this context, the AbstractSparseMatrixCSC interface (getcolptr etc. ) is currently used as a matter of fact though it is not exported or documented as such.

IMHO, both levels of interfaces should (and can) coexist.

j-fu avatar Aug 21 '24 14:08 j-fu