More remarks on the code

[gdalle]

GPUGraphs.jl/src/GPUGraphs.jl

Line 14 in 38d2dc7

include("algebra.jl")

GPUGraphs.jl/src/GPUGraphs.jl

Line 19 in 38d2dc7

include("graphBLAS.jl")

These files don't exist. #2 would have caught it.

GPUGraphs.jl/src/GPUGraphsVector.jl

Line 27 in 38d2dc7

backend::B

We don't need to store the backend, we can retrieve it when we need it with get_backend.

and checking backend consistency between vectors at construction time.

GPUGraphs.jl/src/GPUGraphsVector.jl

Lines 76 to 85 in 38d2dc7

	function SparseGPUVector(
	n::Int,
	::Type{Tv},
	::Type{Ti},
	backend::Backend,
	) where {Tv,Ti<:Integer}
	nzind = allocate(backend, Ti, 0)
	nzval = allocate(backend, Tv, 0)
	SparseGPUVector(n, nzind, nzval, backend)
	end

Why allocate 0 instead of n?

GPUGraphs.jl/src/GPUGraphsVector.jl

Line 90 in 38d2dc7

Base.size(V::SparseGPUVector) = V.n

Base.size returns a tuple, even on vectors.

GPUGraphs.jl/src/GPUGraphsVector.jl

Line 98 in 38d2dc7

pos = findfirst(V.nzind .== i)

You can replace this with a functional variant to avoid allocation (at least on CPUs it's more efficient):

findfirst(==(i), V.nzind)

GPUGraphs.jl/src/GPUGraphsMatrix.jl

Line 9 in 38d2dc7

SparseGPUMatrixCSR{Tv,Ti<:Integer} <: AbstractSparseMatrixCSC{Tv,Ti}

Not a CSC matrix.

[AntoineBut]

In the same order:

I fixed the imports and set up a proper CI. A weird error is thrown when I test the multiplication on KA's CPU backend (someone already opened an issue here), I am not sure what to do about this, but it prevents the test from runnin on the CI.

I agree we can remove the backend field from my structs, will get on it soon.

In the empty constructors, I allocate vectors of length 0 as these vectors store the non-zero values of the matrix / vector, of which there are none. This is still a "work in progress" area as there is no way to add new non-zero elements to SparseGPUArrays, so the empty constructors are useless anyway.

Thank you for the last 3 remarks, I haven't made the changes yet but will do too. I'll close the issue when these are all implemented.

[gdalle]

@AntoineBut can we close this one?

[AntoineBut]

@gdalle yes, I think all the remarks mentioned here have been addressed.