Skip to content

RFC: Add rowval index type to SparseMatrixCSC #39899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Mar 3, 2021

This allows to have distinct "index types" for the nonzeros on the one hand, and the rowvals on the other hand. In general, this may not be a dramatic reduction in memory usage, but in some cases this may push limits, see #38790. This should be fully backward compatible, and basically all existing tests pass up to minor obvious/inavoidable changes.

TODO (if we agree to go in this direction):

  • new tests
  • update documentation
  • mention in NEWS

Closes #38790.

@dkarrasch dkarrasch added the sparse Sparse arrays label Mar 3, 2021
@fredrikekre
Copy link
Member

This should be fully backward compatible

All the changes you had to do, e.g. Symmetric{T,SparseMatrixCSC{T,SuiteSparse_long}} -> Symmetric{T,<:SparseMatrixCSC{T,SuiteSparse_long}} suggest it is not though.

@dkarrasch
Copy link
Member Author

I see, thanks for clarifying. I had in mind the call to constructors, the assumption that Ti == Tr and Ti == Int by default, and that the new Tr is the trailing parameter which doesn't require specification... except for the wrapper cases, as you point out.

@ViralBShah
Copy link
Member

It is possible this could be quite breaking. We should run pkgeval. Also, does it increase compile time noticeably?

@ViralBShah
Copy link
Member

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@dkarrasch
Copy link
Member Author

dkarrasch commented Mar 22, 2021

I checked a few, and the test failures are related to things that match my modification pattern Fredrik pointed out. So I believe these 33- packages (some fail for unknown reasons) could be changed in a way that doesn't require version-checking and alike, and then should work also with previous Julia versions. Is this something we want to consider?

@fredrikekre
Copy link
Member

fredrikekre commented Mar 22, 2021

There is also the cases of, for example,

struct X
    m::SparseMatrixCSC{Float64,Int}
end

that will now be abstractly typed.

It seems like a very niche usecase (?) so perhaps it could just live in a package? After #33039 it might not even require much more code than simply

struct SparseMatrixCSC2{Tv,Ti<:Integer,Tr<:Integer} <: SparseArrays.AbstractSparseMatrixCSC{Tv,Ti}
    m::Int
    n::Int
    colptr::Vector{Ti}
    rowval::Vector{Tr}
    nzval::Vector{Tv}
end

@dkarrasch
Copy link
Member Author

@msekino Could you check if Fredrik's suggestion above works for you? If it can be done without much effort in an extra package (or just as part of your package), then I now agree that this is the way to go, since the cost of adaptation throughout the ecosystem is rather high for such a change here.

@msekino
Copy link

msekino commented Mar 23, 2021

@dkarrasch @fredrikekre
Thank you for your consideration!
I think that the suggestion is a simple and good solution and will work for us without much effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The eltype of SparseMatrixCSC's colptr should be Int instead of Ti
5 participants