AbstractSparseMatrixCSC not exported

[j-fu]

Hi, I would like to use the AbstractSparseMatrixCSC interface with my sparse matrix struct, however it seems that it is not exported.

Also, it is not completely clear what needs to be implemented to conform to the interface.
I currently assume it is getcolptr, getrowval, nonzeros, nnz, size.

[rayegun]

That should be sufficient for most operations yes. There will definitely be some things missing, there are not many other subtypes of AbstractSparseMatrixCSC in the ecosystem to test it out.

You can qualify the name SparseArrays.AbstractSparseMatrixCSC, but I'll look into adding it to exports.

[j-fu]

... bump...

[ViralBShah]

Is it possible to provide a PR? We should also document the interface.

[j-fu]

Ok will try. Can get on this in a couple of days.

[j-fu]

What the sparse(A) method should do ?
Currently, we have

SparseArrays.jl/src/sparsematrix.jl

Line 1000 in 0d2d30a

sparse(S::AbstractSparseMatrixCSC) = copy(S)

and

SparseArrays.jl/src/sparsematrix.jl

Line 729 in 0d2d30a

SparseMatrixCSC(S::AbstractSparseMatrixCSC) = copy(S)

(and following).

All of them copy the AbstractSparseMatrixCSC. IMHO all of them could return a SparseMatrixCSC via this constructor,
and I think the wordl would be a better place if they would.

So the question here is: is a sparse(A::AbstractSparseMatrixCSC)::SparseMatrixCSC method to be seen as a part of the AbstractSparseMatrixCSC interface ?

[ViralBShah]

If you have your own concrete implementation of AbstractSparseMatrixCSC, then sparse should give you a copy of the matrix in that concrete type, I believe. That way, I believe, your type will propagate as much as possible through the implementation.

[j-fu]

Ok, that may be the philosophy behind sparse. But then why currently some of the constructors of SparseMatricCSC from AbstractSparseMatrixCSC do copy too ?

[ViralBShah]

The AbstractSparseMatrixCSC was not in the original design, but introduced later on by folks who want to roll out their own. So it was a mechanism for hooking in by whoever needed it last and needs to keep improving.

[j-fu]

... this is exactly my reason to try to help to move this forward :)

[j-fu]

But still: why do we need these ?

SparseArrays.jl/src/sparsematrix.jl

Line 729 in 0d2d30a

SparseMatrixCSC(S::AbstractSparseMatrixCSC) = copy(S)

SparseArrays.jl/src/sparsematrix.jl

Line 733 in 0d2d30a

SparseMatrixCSC{Tv,Ti}(S::AbstractSparseMatrixCSC{Tv,Ti}) where {Tv,Ti} = copy(S)

In both cases, one would expect a SparseMatrixCSC as a result and not whichever else subtype of AbstractSparseMatrixCSC...

EDIT: @tkf, @KlausC

[ViralBShah]

Yes, for the SparseMatrixCSC constructor, it should be a SparseMatrixCSC, but for sparse, it should be available to the new abstract sparse type, right?

BTW, I always felt it was a bit weird to have AbstractSparseMatrixCSC and the right thing would have been to have AbstractSparseMatrix.

[j-fu]

So a PR should fix this ?

BTW, I always felt it was a bit weird to have AbstractSparseMatrixCSC and the right thing would have been to have AbstractSparseMatrix.

... I can understand that one - there is at least the SparseMatrixCSR storage scheme which is also an AbstractSparseMatrix:

https://github.com/gridap/SparseMatricesCSR.jl/blob/d26c461eefbd27550529e06e2cafae24265657ea/src/SparseMatrixCSR.jl#L28

[KlausC]

But still: why do we need these ?

I agree with 729, 731, 733 are wrong; they should all fall back to what is defined in line 734

For line 1000 I would prefer sparse(s::Abstract...) = s. Why make a copy, it it is already sparse? If you need a copy use copy,

[j-fu]

For sparse I would argue to return a SparseMatrixCSC as well, but legacy may suggest otherwise - I guess we would need to hear what e.g. @tkf says (git blame says that he wrote this). After all this may be breaking for some.

[SobhanMP]

For sparse I would argue to return a SparseMatrixCSC as well, but legacy may suggest otherwise

I think it's a good to just expect an AbstractMatrixCSC, changing it will break things, probably fixedsparse. The point of the sparse function is to just return something sparse. I think the copy is there for safety so that the returned matrix can be edited. This way the returned matrix can be edited for both dense and sparse matrices. Also at the end of the day, sparse(s::Abstract) is just a fallback, types that cannot be edited can and should overload it.

[rayegun]

BTW, I always felt it was a bit weird to have AbstractSparseMatrixCSC and the right thing would have been to have AbstractSparseMatrix.

I tend to support an immediate abstract type for matrix types. For instance a SparseMatrixCSC with the addition of a lazy insertion capability is still an AbstractSparseMatrixCSC, and should by and large use the same methods. The fixed stuff for instance uses this AFAIR.

[j-fu]

...probably fixedsparse...

fixedsparse: does this refer to some particular package ?

[SobhanMP]

It's in this package ... FixedSparseCSC

[j-fu]

Please give a link - I don't find it...

[SobhanMP]

SparseArrays.jl/src/sparsematrix.jl

Line 62 in 1c46709

struct FixedSparseCSC{Tv,Ti<:Integer} <: AbstractSparseMatrixCSC{Tv,Ti}

[SobhanMP]

anyhow point is, sparse for already sparse matrices works, if you need SparseMatrixCSC, just call SparseMatrixCSC on the thing.

[KlausC]

the interface.
I currently assume it is getcolptr, getrowval, nonzeros, nnz, size

I think, we should not forget nzrange, dropzeros!. getcolptr is maybe not so important. nnz and size are included in AbstractArray already. nonzeros is ok and getnzval should be deprecated. I would prefer something more "abstract" than getrowval.

[j-fu]

From my point of view it should be possible to construct a SparseMatrixCSC from this interface, see e.g. https://github.com/SciML/LinearSolve.jl/blob/main/src/factorization.jl#L349

[SobhanMP]

I think you don't even need to do that, see

SparseArrays.jl/src/sparsematrix.jl

Line 734 in d938744

function SparseMatrixCSC{Tv,Ti}(S::AbstractSparseMatrixCSC) where {Tv,Ti}

,
calling SparseMatrixCSC, should just "work".

[KlausC]

There is already an interface nnz - nzrange - nonzeros - rowvals established for AbstractMatrixCSC and SparseVector.
So getrowval is obsoleted by rowvals. getcolptr can mostly replaced by nzrange.

[j-fu]

Yes, the interface is established but not really documented. As I said in the start of the issue thread I will try to make PR to fix this (including exporting AbstractSparseMatrixCSC), which should also fix the errors in lines 729, 731, 733 of sparsematrix.jl, just need to find time to focus on this (not before mid of next week...)

I agree upon nnz - nzrange - nonzeros - rowvals but I also would like to see getcolptr a part of it - this would be needed to pass data to whatever else non-Julia implementation.

I would keep sparse() as it is now after the discussion in this thread.

[KlausC]

See also #52 and the discussions referred to there

[j-fu]

IMHO it seems it is too late now to rename or remove getcolptr as rg getcolptr in .julia/packages reveals.

Though formally it cannot be breaking, the resulting work effort IMHO would be not worth it - it appears to me that it is more important to stabilize the API here.