Skip to content

Conversation

@dkarrasch
Copy link
Member

@dkarrasch dkarrasch added backport 1.7 bugfix This change fixes an existing bug linear algebra Linear algebra labels Jun 23, 2021
@maleadt
Copy link
Member

maleadt commented Jun 23, 2021

Thanks! Disregarding the CI failures here, this does make the relevant CUDA.jl code work again.

@andreasnoack
Copy link
Member

The problem is that similar(::SparseMatrixCSC) is a SparseMatrixCSC. We need the output of _zeros to be a dense array.

@dkarrasch
Copy link
Member Author

So we need to specialize _zeros(::Type{T}, b::AbstractSparseVector, n::Integer) to give a dense vector, right? I'll add that in SparseArrays and see how it goes.

@andreasnoack
Copy link
Member

The problem is that _zero was supposed to be a single simple helper function. With these changes, it almost becomes part of the interface. I'm not sure what the right solution is. This wouldn't be a problem if similar(::SparseMatrixCSC) returned a Matrix.

@dkarrasch
Copy link
Member Author

How did it work earlier? How did we use similar yet avoided the *Diagonal and sparse issues?

@andreasnoack
Copy link
Member

How did it work earlier?

I think that we effectively called zeros or Matrix for these cases. For some reason it didn't cause issues for CUDA. In my PR, I made all \ use the same code path so a few extra \ might now use zeros/Matrix instead of similar. Maybe Cholsky\... since @maleadt mentioned potrs. Maybe a better solution is to add \ method to CUDA similar to the one here that initializes the rhs with a CuArray. We don't really have the right generic function to initialize the right hand side of a solve. At least that would require changing similar for sparse arrays in a way that probably isn't feasible at this stage.

@maleadt
Copy link
Member

maleadt commented Jun 25, 2021

Maybe Cholsky\... since @maleadt mentioned potrs.

Yup, that was the one. I'll duplicate a version of \ for CuArrays in CUDA.jl.

@dkarrasch
Copy link
Member Author

So we can close this, right?

@andreasnoack
Copy link
Member

I'd think so. @maleadt can you please confirm that you were able to work around the issue by adding a method to CUDA.jl?

@maleadt
Copy link
Member

maleadt commented Jun 28, 2021

Sorry for being unclear, yes this can be closed. It would be nice to revisit this at some point though, because code like https://github.com/JuliaGPU/CUDA.jl/blob/8ba1fe4f1c2b94e309eb4c51583783a43e7f297d/src/linalg.jl#L17-L48 is pretty fragile.

@maleadt maleadt closed this Jun 28, 2021
@DilumAluthge DilumAluthge deleted the dk/_zeros branch August 24, 2021 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This change fixes an existing bug linear algebra Linear algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants