-
Notifications
You must be signed in to change notification settings - Fork 29
Fix sparse gemm, gemv, and mul #536
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
base: master
Are you sure you want to change the base?
Conversation
ca73b5f
to
4645ffd
Compare
lib/mkl/wrappers_sparse.jl
Outdated
(:onemklZsparse_set_csr_data_64, :ComplexF64, :Int64)) | ||
@eval begin | ||
|
||
function oneSparseMatrixCSR( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also have a similar constructor for oneSparseMatrixCOO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I didn't want to add the constructor changes in this commit. I'll remove those.
4645ffd
to
570e801
Compare
lib/mkl/wrappers_sparse.jl
Outdated
(:onemklZsparse_set_csr_data_64, :ComplexF64, :Int64)) | ||
@eval begin | ||
|
||
function oneSparseMatrixCSR( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a similar constructor for oneSparseMatrixCOO
?
# Matrix-vector multiplication with transpose/adjoint | ||
function LinearAlgebra.generic_matvecmul!(C::oneVector{T}, tA::AbstractChar, A::Transpose{T, <:oneSparseMatrixCSR{T}}, B::oneVector{T}, _add::MulAddMul) where {T <: BlasFloat} | ||
tA = tA in ('S', 's', 'H', 'h') ? 'N' : tA | ||
tA_final = tA == 'N' ? 'T' : (tA == 'T' ? 'N' : 'C') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michel2323 In the case tA = 'C'
, you need to do a product with the conjugate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if you didn't mixed BlasReal
with BlasFloat
?
|
||
function LinearAlgebra.generic_matvecmul!(C::oneVector{T}, tA::AbstractChar, A::Transpose{T, <:oneSparseMatrixCSC{T}}, B::oneVector{T}, _add::MulAddMul) where {T <: BlasFloat} | ||
tA = tA in ('S', 's', 'H', 'h') ? 'N' : tA | ||
tA_final = tA == 'N' ? 'T' : (tA == 'T' ? 'N' : 'C') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar issue, you need a special handle of tA = 'C'
.
|
||
function LinearAlgebra.generic_matvecmul!(C::oneVector{T}, tA::AbstractChar, A::Transpose{T, <:oneSparseMatrixCOO{T}}, B::oneVector{T}, _add::MulAddMul) where {T <: BlasFloat} | ||
tA = tA in ('S', 's', 'H', 'h') ? 'N' : tA | ||
tA_final = tA == 'N' ? 'T' : (tA == 'T' ? 'N' : 'C') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again, special handle of tA = 'C'
is needed (when the matrix is complex).
570e801
to
5c833a7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #536 +/- ##
==========================================
- Coverage 79.20% 77.08% -2.13%
==========================================
Files 47 47
Lines 2996 3168 +172
==========================================
+ Hits 2373 2442 +69
- Misses 623 726 +103 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.