Skip to content
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

Refactoring: Add AbstractSparseMatrixCSC #33039

Merged
merged 4 commits into from
Aug 24, 2019

Conversation

tkf
Copy link
Member

@tkf tkf commented Aug 23, 2019

Now that we have accessor-based code to manipulate sparse arrays #32953, we can expose the rich set of functions in SparseArrays.jl in a highly extensible manner by "just" replacing SparseMatrixCSC with AbstractSparseMatrixCSC in appropriate places. This is the second step I discussed with @ViralBShah in #30173 (comment).

Quick summary

This patch introduces a new abstract type AbstractSparseMatrixCSC

AbstractSparseMatrixCSC{Tv,Ti<:Integer} <: AbstractSparseMatrix{Tv,Ti}
Supertype for matrix with compressed sparse column (CSC).

and then declare SparseMatrixCSC to be the subtype of it

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

I then apply a series of diffs like below so that the functions previously dispatched on SparseMatrixCSC is now dispatched on AbstractSparseMatrixCSC

@@ -31,7 +31,7 @@ end
 # In matrix-vector multiplication, the correct orientation of the vector is assumed.
 const AdjOrTransStridedMatrix{T} = Union{StridedMatrix{T},Adjoint{<:Any,<:StridedMatrix{T}},Transpose{<:Any,<:StridedMatrix{T}}}
 
-function mul!(C::StridedVecOrMat, A::SparseMatrixCSC, B::Union{StridedVector,AdjOrTransStridedMatrix}, α::Number, β::Number)
+function mul!(C::StridedVecOrMat, A::AbstractSparseMatrixCSC, B::Union{StridedVector,AdjOrTransStridedMatrix}, α::Number, β::Number)
     size(A, 2) == size(B, 1) || throw(DimensionMismatch())
     size(A, 1) == size(C, 1) || throw(DimensionMismatch())
     size(B, 2) == size(C, 2) || throw(DimensionMismatch())

@ViralBShah ViralBShah added the domain:arrays:sparse Sparse arrays label Aug 23, 2019
@ViralBShah
Copy link
Member

This also largely looks ok to me. Should be completely non-breaking, so can merge when we are ready.

@tkf tkf mentioned this pull request Aug 24, 2019
4 tasks
@tkf tkf changed the title Add AbstractSparseMatrixCSC Refactoring: Add AbstractSparseMatrixCSC Aug 24, 2019
@tkf
Copy link
Member Author

tkf commented Aug 24, 2019

I'd appreciate it if this PR can be merged relatively quickly to avoid interference with other PRs. I opened PR #33054 for finalizing the interface definition and prefer to do a discussion on the interface there (including naming bikeshedding (if we have to...)). Note that renaming AbstractSparseMatrixCSC afterward should be very easy since this name is unique in this repository.

@ViralBShah ViralBShah merged commit da9685b into JuliaLang:master Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants