Skip to content
This repository was archived by the owner on Mar 12, 2021. It is now read-only.

Fix sparse mul! #692

Merged
merged 3 commits into from
Apr 27, 2020
Merged

Fix sparse mul! #692

merged 3 commits into from
Apr 27, 2020

Conversation

amontoison
Copy link
Member

@maleadt
Copy link
Member

maleadt commented Apr 23, 2020

Thanks!
bors r+

bors bot added a commit that referenced this pull request Apr 23, 2020
@bors @amontoison
Merge #692
991ecf8 
692: Fix sparse mul! r=maleadt a=amontoison

close #629 
#630 , #637 
@haampie 

Co-authored-by: Alexis Montoison <[email protected]>
@bors
Copy link
Contributor

bors bot commented Apr 23, 2020

Build failed:

@amontoison
Copy link
Member Author

Tests failed with A' * x when A is a CuSparseMatrixCSC{ComplexF32} or a CuSparseMatrixCSC{ComplexF64}. All tests passed with CuSparseMatrixCSR{...}.

@amontoison amontoison force-pushed the adjoint-sparse-mul-fix branch 2 times, most recently from faca527 to dd44454 Compare April 23, 2020 23:38
@amontoison
Copy link
Member Author

The culprit should be mv! : https://github.com/JuliaGPU/CuArrays.jl/blob/master/src/sparse/wrappers.jl#L554-L584

@amontoison
Copy link
Member Author

amontoison commented Apr 23, 2020 •
edited
Loading

The conjugate-transpose of a transposed complex matrix A is not A but conj(A), it's why tests failed with complex matrices.

@amontoison amontoison force-pushed the adjoint-sparse-mul-fix branch 2 times, most recently from 97c71dd to 713422b Compare April 26, 2020 20:06
@amontoison
Copy link
Member Author

I read the documentation of csrmv function and it seems that they forgot this corner case.
https://docs.nvidia.com/cuda/cusparse/index.html#csrmv

@maleadt, it's ready to be tested.

amontoison
amontoison commented Apr 26, 2020
View reviewed changes
src/sparse/wrappers.jl
@@ -567,17 +567,22 @@ for (fname,elty) in ((:cusparseScsrmv, :Float32),
ctransa = 'T'
end
cutransa = cusparseop(ctransa)
cuind = cusparseindex(index)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cuind variable is not used. I removed it.

@amontoison amontoison force-pushed the adjoint-sparse-mul-fix branch from 713422b to 2ec414f Compare April 26, 2020 20:13
@maleadt
Copy link
Member

maleadt commented Apr 27, 2020

bors d+

@bors
Copy link
Contributor

bors bot commented Apr 27, 2020

✌️ amontoison can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@maleadt
Copy link
Member

maleadt commented Apr 27, 2020

bors try

bors bot added a commit that referenced this pull request Apr 27, 2020
@bors
Try #692:
602e5fc
@maleadt
Copy link
Member

maleadt commented Apr 27, 2020

What about:

[[DEPRECATED]] use cusparseSpMV() instead. The routine will be removed in the next major release

@bors
Copy link
Contributor

bors bot commented Apr 27, 2020

try

Build failed:

@maleadt
Copy link
Member

maleadt commented Apr 27, 2020

bors try

bors bot added a commit that referenced this pull request Apr 27, 2020
@bors
Try #692:
bd4aa17
@bors
Copy link
Contributor

bors bot commented Apr 27, 2020

try

Build succeeded:

@amontoison
Copy link
Member Author

About the DEPRECATED csrmv, it concerned numerous functions in CUSPARSE module. 😟

@amontoison
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 27, 2020

Build succeeded:

@bors bors bot merged commit 16c0080 into JuliaGPU:master Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mul! failing for adjoint sparse matrices
2 participants
@amontoison @maleadt