Calling similar(...) on AbstractCuSparseArray with dims #1184

birkmichael · 2021-10-04T15:52:24Z

Calling similar(var,dims) or similar(var,type,dims) on a variable var with typeof(var):<AbstractCuSparseArray will return a CuArray of eltype eltype(var) with dimensions given by dims. Previously, calling similar(reshape(var,dims)) would result in Base.abstractarray.jl and Base.reshapedarray.jl expanding the call to similar(reshape(var,dims),eltype,dims), which was not handled in this code and resulted in a call to Array(...) and not CuArray(...), forcing use of CPU and scalar indexing.

Calling similar() on a variable var of type :<AbstractCuSparseArray along with dims argument will return a CuArray of type eltype(var) with dimensions given by dims.

calling similar on AbstractCuSparseArray with dims

maleadt · 2021-10-04T16:00:16Z

Thanks for the contribution! I'm not sure I fully understand the fix though, shouldn't similar on a sparse array yield another sparse array, not a CuArray?

julia> similar(sprand(2,2,0.5))
2×2 SparseMatrixCSC{Float64, Int64} with 3 stored entries:
 6.90942e-310  6.90965e-310
 6.90941e-310   ⋅ 

julia> similar(cu(sprand(2,2,0.5)))
2×2 CuSparseMatrixCSC{Float32, Int32} with 2 stored entries:
  ⋅    ⋅ 
 0.0  1.875

Furthermore, a wrapped array isn't a subtype of the original array:

julia> reshape(cu(sprand(2,2,0.2)), 2, 2, 1) isa CUSPARSE.AbstractCuSparseArray
false

I've also marked your PR as draft so that it doesn't kick off unnecessary CI. It would need some tests for that to be useful anyway.

lib/cusparse/array.jl

birkmichael · 2021-10-04T17:07:56Z

Furthermore, a wrapped array isn't a subtype of the original array:

When similar is called on a reshaped array (CPU or GPU), parent is called to unwrap, and then similar is called on the unwrapped array. This is good behavior for GPU arrays as well. The problem is that when the call is made on the unwrapped array, eltype and dims are added as arguments, which is currently not handled by CuSparse, so that similar is called from Base and generates an Array rather than a CuArray.

shouldn't similar on a sparse array yield another sparse array, not a CuArray?

You're right. I thought I was mimicing julia's regular sparse behavior, but I just realized how and why similar(CPUSparseVar,dims::Dims{N>=3}) returns a CPUArray. I will fix my branch and resubmit.

Added more robust handling of applying similar on a CuSparseMatrixCSC/CSR, following the logic used in SparseArrays.jl

robust handling of similar CSC/CSR

two methods CSR were originally added as extensions to SparseArrays.jl, but SparseArrays.jl does not support CSR.

fixed getrowptr,colvals

forgot Base. prefix to several instances of similar

used fill instead of CUDA.fill in matrix initialization

codecov · 2021-10-04T21:36:10Z

Codecov Report

Attention: Patch coverage is 86.30137% with 10 lines in your changes missing coverage. Please review.

Project coverage is 80.50%. Comparing base (dae1e18) to head (3b64e56).
Report is 1125 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/cusparse/array.jl	83.33%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1184      +/-   ##
==========================================
+ Coverage   80.48%   80.50%   +0.02%     
==========================================
  Files         119      119              
  Lines        8393     8454      +61     
==========================================
+ Hits         6755     6806      +51     
- Misses       1638     1648      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

unclear what is supported in CUDA.jl for BSR,COO, so they remain untested.

Cu sparse Base.similar tests completed for CSC and CSR. No tests for BSR, COO, as it is unclear to what extent these are supported in CUDA.jl.

splatted 1D tuple dims when creating vector

birkmichael · 2021-10-06T22:25:12Z

@maleadt So it looks like the tests failed, but not because of me. Some error in the comparison Array(dx[:])==x[:] in tests/cusparse.jl.

maleadt · 2021-10-07T08:08:47Z

So it looks like the tests failed, but not because of me. Some error in the comparison Array(dx[:])==x[:] in tests/cusparse.jl.

CI on master is green, the failure is in tests/cusparse and this PR is changing stuff with sparse arrays, so...
What makes you say the failure is not related?

Added method for colon indexing a CuSparseMatrix, and a conversion from CuSparseMatrix to CuSparseMatrix to vector. Fixed show method of vectors to be same as for SparseArrays. CuSparseVector.dims is a 2-tuple instead of an integer, probably as a holdover from the base.show method that was written to fit both vectors and matrices, and needed to be able to run 'size(A)[2]'. That can probably be changed now, but I don't want to break anyone's code.

birkmichael · 2021-10-09T12:48:39Z

@maleadt It looks like there were a bunch of tests that did indexing of a CuSparseMatrix and compared them to a CPU sparse matrix, a la Array(d_x[inds])==x[inds]. All of these worked in master because indexing with just a single colon or with ranges/vectors just turned the CuSparseMatrix into a dense CPU array, and then indexed into that (even before Array() was called). Now that similar is fixed to return the correct type of array, the lack of specialized methods for indexing CuSparseMatrix variables is causing errors. I'm thinking of solving all of these bugs just by converting to a sparse CPU array, letting SparseArrays do its magic, and then convert back to a CuSparseMatrix. It looks like these operations demand scalar indexing anyways, so it's not much of a performance loss. What do you think?

maleadt · 2021-10-12T14:08:28Z

Copying an entire sparse array to the CPU when you only want to access a single element seems excessive... Scalar iteration is slow because it performs a memcpy for every element, that doesn't mean we should make it even slower by memcpying the entire array for each access.

birkmichael · 2021-10-12T14:32:54Z

It's not for a single element. Specifically, what exists now (and returns the right type):

indexing with a single integer and a colon (both arrangements - row slicing and column slicing)
indexing with two colons
indexing with two integers
indexing with a single colon - added in my PR, converts to a CPU sparse array, uses their algorithm to convert to a sparse vector (with scalar iteration), and then converts to a CuSparseVector.

What I'm talking about is indexing where one or both of the indices is a vector or range. In master, if one is a vector/range, you will get a cpu array. If both are, you will get an error (which is why the test failed). The simplest fix is by converting to CPU and back. Admittedly, this is probably not the most efficient way to go about adding those features, but I'm not sure there is a good method to do this kind of slicing without doing scalar iteration anyway, which is why I am proposing this method.

Cu sparse similar matrix indexing

updating to match current master branch

Similar, indexing CuSparseArrays, Vector reshaping

birkmichael · 2021-10-22T21:56:52Z

All tests passed except those that fail master. Added a bunch of new tests (tests/cusparse/arrays.jl), so the coverage will probably improve. Anyway, now you can index and slice CuCSC and CuCSR matrices freely and they will stay the same type, rather than drop to the CPU. Any type of AbstractCuSparseMatrix can be turned into a CuSparseVector now. Everything is done using (almost only) vectorised methods without going through the CPU.

maleadt · 2021-10-25T15:16:08Z

lib/cusparse/array.jl

@@ -200,6 +200,59 @@ Base.similar(Mat::CuSparseMatrixCSC) = CuSparseMatrixCSC(copy(Mat.colPtr), copy(
 Base.similar(Mat::CuSparseMatrixCSR) = CuSparseMatrixCSR(copy(Mat.rowPtr), copy(Mat.colVal), similar(nonzeros(Mat)), Mat.dims)
 Base.similar(Mat::CuSparseMatrixBSR) = CuSparseMatrixBSR(copy(Mat.rowPtr), copy(Mat.colVal), similar(nonzeros(Mat)), Mat.blockDim, Mat.dir, nnz(Mat), Mat.dims)

+## similar for CSC,CSR with dims, eltype arguments, adapted from SparseArrays.jl. NOTE: calling similar() on a wrapped CuSparseMatrixBSR/COO will result in a CuArray.


Maybe wrap the NOTE on a new line.

But also: why does similar still result in a dense vector? #1184 (comment)

It shouldn't - I spent a lot of time on that. I'll check.

maleadt

Thanks! I don't have the time for an in depth review right now, so some quick comments.

maleadt · 2021-10-25T15:16:47Z

lib/cusparse/array.jl

+
+# Handles cases where CSC/CSR are reshaped to more than two dimensions, and all cases above for COO/BSR.
+Base.similar(a::AbstractCuSparseArray, ::Type{T}, dims::Base.Dims{N}) where {T,N} =
+CuArray{T,N}(undef, dims)               


indent?

Comment from above also applies.

maleadt · 2021-10-25T15:17:36Z

lib/cusparse/array.jl

-    r1 = convert(Int, x.colPtr[j])
-    r2 = convert(Int, x.colPtr[j+1]) - 1
-    CuSparseVector(rowvals(x)[r1:r2], nonzeros(x)[r1:r2], size(x, 1))
+Base.getindex(A::CuSparseMatrixCSC,I::AbstractVector,J::AbstractVector) = CuSparseMatrixCSC(getindex(CuSparseMatrixCSR(getindex(A,:,J)),I,:))


Maybe wrap these two methods for legibility.

How do you mean? Write a method for Base.getindex(A::CuSparseMatrixCSCR,I::AbstractVector,J::AbstractVector) that calls either of these two methods, depending on type?

No, just line wrapping :-) i.e. adding a newline after the =

maleadt · 2021-10-25T15:18:10Z

lib/cusparse/array.jl

+    newcolptr = cumsum(diffVec)
+
+    indices = map((x,y)->range(x,stop=y),colptr[I_sorted],colptr[I_sorted.+1].-1)
+    @CUDA.allowscalar indices = vcat(indices...) #TODO: figure out how to avoid this step


CUDA module prefix shouldn't be needed.

But the scalar indexing is not good. where does it come from?

What I'm trying to do here is this: get an array indices of vectors, where indices[i] contains a vector of the indices of all elements in nzvals that are in the I_sorted[i] row (or column, depending on CSC or CSR) of the input matrix. After that, indexing becomes simple.

However, the map here returns a CuArray of ranges, and any indexing I do with that will cause a bug, so I need to turn the ranges into vectors first. If I had a method to get a CPU array of CuVectors from the map, instead of a CuArray of ranges, that would be unnecessary. Applying collect.(indices) gives me an "inline variables only" type of error because it tries to instantiate a CuArray of arrays of (sometimes) different sizes. Applying vcat(indices...) gives a scalar indexing error.

The other, more elegant method of solving this, is actually doing what I did in conversions.jl that you commented on - transforming colptr to a CPU array, in which case I can just get an array of arrays using a comprehension or a map.

the map here returns a CuArray of ranges, and any indexing I do with that will cause a bug, so I need to turn the ranges into vectors first.

Could you elaborate? And conversions to CPU data is extremely expensive and to be avoided if possible, so I'd rather look into fixing the bug you're encountering.

maleadt · 2021-10-25T15:18:26Z

lib/cusparse/array.jl

@@ -446,6 +535,7 @@ Base.copy(Mat::CuSparseMatrixCOO) = copyto!(similar(Mat), Mat)

 # input/output

+


Some needless whitespace changes?

maleadt · 2021-10-25T15:18:30Z

lib/cusparse/array.jl

@@ -483,7 +573,6 @@ for (gpu, cpu) in [CuSparseMatrixCSC => SparseMatrixCSC,
    end
 end

-


Some needless whitespace changes?

maleadt · 2021-10-25T15:25:55Z

test/cusparse/array.jl

@@ -0,0 +1,143 @@
+using CUDA.CUSPARSE
+using SparseArrays
+using Test


You shouldn't need to import Test -- run select tests by doing julia -L test/setup.jl test/cusparse/array.jl

maleadt · 2021-10-25T15:28:10Z

lib/cusparse/conversions.jl

+    I = 1:m
+    nzval = nonzeros(Mat)
+    nzind = rowvals(Mat)
+    colptr = Array(getcolptr(Mat)) #TODO: figure out how to avoid this step


this seems bad. there's no API call for such conversions? or maybe try doing the conversion on the GPU to avoid synchronizing during the copy to/from the CPU?

See my comment Re: scalar indexing above - there I try doing it on the GPU and end up doing scalar indexing. Do you mean API calls to the NVidia CUDA library? I had a look now and couldn't find it, but I will give it a bit more effort later.

calling similar on AbstractCuSparseArray with dims

cc65094

Calling similar() on a variable var of type :<AbstractCuSparseArray along with dims argument will return a CuArray of type eltype(var) with dimensions given by dims.

birkmichael mentioned this pull request Oct 4, 2021

similar malfunctions when used on singly-wrapped CuSparseMatrix types #1179

Open

Merge pull request #1 from birkmichael/CuSparse_Similar

ad2c4a4

calling similar on AbstractCuSparseArray with dims

maleadt marked this pull request as draft October 4, 2021 16:00

maleadt added bugfix This gets something working again. cuda array Stuff about CuArray. needs tests Tests are requested. labels Oct 4, 2021

maleadt reviewed Oct 4, 2021

View reviewed changes

lib/cusparse/array.jl Outdated Show resolved Hide resolved

birkmichael added 8 commits October 4, 2021 22:18

robust handling of similar CSC/CSR

2af05e4

Added more robust handling of applying similar on a CuSparseMatrixCSC/CSR, following the logic used in SparseArrays.jl

Merge pull request #2 from birkmichael/CuSparse_Similar

25ad0e1

robust handling of similar CSC/CSR

fixed getrowptr,colvals

53724ce

two methods CSR were originally added as extensions to SparseArrays.jl, but SparseArrays.jl does not support CSR.

Merge pull request #3 from birkmichael/CuSparse_Similar

8c7a33e

fixed getrowptr,colvals

forgot Base. prefix to several instances of similar

6a8b64b

Merge pull request #4 from birkmichael/CuSparse_Similar

ad222bf

forgot Base. prefix to several instances of similar

used fill instead of CUDA.fill in matrix initialization

2620166

Merge pull request #5 from birkmichael/CuSparse_Similar

14149a2

used fill instead of CUDA.fill in matrix initialization

birkmichael marked this pull request as ready for review October 4, 2021 21:30

birkmichael added 8 commits October 5, 2021 21:54

tests for similar, unfinished

05c85ef

Created test set for 'similar'

309f02e

imported some SparseArrays methods

6ec07de

Fixed CSR rowPtr setting in 'similar'

fb4b253

tests for 'similar' for CSC,CSR

df04acb

unclear what is supported in CUDA.jl for BSR,COO, so they remain untested.

Merge pull request #6 from birkmichael/CuSparse_Similar_tests

6956881

Cu sparse Base.similar tests completed for CSC and CSR. No tests for BSR, COO, as it is unclear to what extent these are supported in CUDA.jl.

splatted 1D tuple dims when creating vector

4bf8e00

Merge pull request #7 from birkmichael/vectorfix

2d5b613

splatted 1D tuple dims when creating vector

birkmichael added 4 commits October 9, 2021 12:54

Added method getindex(CuSparseMat,Colon), changed show for vectors

1a097db

Added conversion from matrix to vector

38cf586

Merge branch 'master' into CuSparse_Similar

9b1f529

maleadt force-pushed the master branch from b4a4542 to 0e9ad9d Compare October 13, 2021 15:32

birkmichael added 13 commits October 22, 2021 02:27

Full indexing of CuSparseArrays

575b576

Vectorising matrices

bd16d76

Merge branch 'JuliaGPU:master' into master

dc4075f

Added vectorising of CuSparseMatrices

69e69d6

Update array.jl

9a7196f

Update conversions.jl

f90a4d6

Update conversions.jl

0e45e48

Merge pull request #11 from birkmichael/CuSparse_Similar_matrixIndexing

43240b3

Cu sparse similar matrix indexing

changed to match updates in master

e62cbb6

Update conversions.jl

509d10a

Merge pull request #12 from JuliaGPU/master

e47b56b

updating to match current master branch

Merge branch 'CuSparse_Similar' into master

66103d1

Merge pull request #13 from birkmichael/master

3b64e56

Similar, indexing CuSparseArrays, Vector reshaping

birkmichael requested a review from maleadt October 22, 2021 23:04

maleadt reviewed Oct 25, 2021

View reviewed changes

maleadt force-pushed the master branch from db0ecb0 to 4d6dbd1 Compare May 13, 2022 07:37

maleadt force-pushed the master branch from 476979e to d53a63e Compare March 16, 2023 12:34

maleadt force-pushed the master branch from c97bc77 to d57e020 Compare September 8, 2023 20:12

maleadt force-pushed the master branch from 1cb1f53 to 1a1d127 Compare September 18, 2023 16:28

maleadt force-pushed the master branch from aef3298 to 4b017c6 Compare January 18, 2024 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calling similar(...) on AbstractCuSparseArray with dims #1184

Calling similar(...) on AbstractCuSparseArray with dims #1184

birkmichael commented Oct 4, 2021

maleadt commented Oct 4, 2021 •

edited

Loading

birkmichael commented Oct 4, 2021 •

edited

Loading

codecov bot commented Oct 4, 2021 •

edited

Loading

birkmichael commented Oct 6, 2021

maleadt commented Oct 7, 2021

birkmichael commented Oct 9, 2021

maleadt commented Oct 12, 2021

birkmichael commented Oct 12, 2021 •

edited

Loading

birkmichael commented Oct 22, 2021

maleadt Oct 25, 2021

birkmichael Oct 25, 2021

maleadt left a comment

maleadt Oct 25, 2021

maleadt Oct 25, 2021

birkmichael Oct 25, 2021

maleadt Nov 18, 2021

maleadt Oct 25, 2021

birkmichael Oct 25, 2021

maleadt Nov 18, 2021

maleadt Oct 25, 2021

maleadt Oct 25, 2021

maleadt Oct 25, 2021

maleadt Oct 25, 2021

birkmichael Oct 25, 2021

		@@ -446,6 +535,7 @@ Base.copy(Mat::CuSparseMatrixCOO) = copyto!(similar(Mat), Mat)

		# input/output

		@@ -483,7 +573,6 @@ for (gpu, cpu) in [CuSparseMatrixCSC => SparseMatrixCSC,
		end
		end

Calling similar(...) on AbstractCuSparseArray with dims #1184

Are you sure you want to change the base?

Calling similar(...) on AbstractCuSparseArray with dims #1184

Conversation

birkmichael commented Oct 4, 2021

maleadt commented Oct 4, 2021 • edited Loading

birkmichael commented Oct 4, 2021 • edited Loading

codecov bot commented Oct 4, 2021 • edited Loading

Codecov Report

birkmichael commented Oct 6, 2021

maleadt commented Oct 7, 2021

birkmichael commented Oct 9, 2021

maleadt commented Oct 12, 2021

birkmichael commented Oct 12, 2021 • edited Loading

birkmichael commented Oct 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleadt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleadt commented Oct 4, 2021 •

edited

Loading

birkmichael commented Oct 4, 2021 •

edited

Loading

codecov bot commented Oct 4, 2021 •

edited

Loading

birkmichael commented Oct 12, 2021 •

edited

Loading