Skip to content

Fix sparse gemm, gemv, and mul for complex types - #536

Closed
michel2323 wants to merge 1 commit into
mainfrom
ms/sparse_fix
Closed

michel2323 wants to merge 1 commit into
mainfrom
ms/sparse_fix

Conversation

@michel2323

Copy link
Copy Markdown
Member

No description provided.

Comment thread lib/mkl/wrappers_sparse.jl Outdated
(:onemklZsparse_set_csr_data_64, :ComplexF64, :Int64))
@eval begin

function oneSparseMatrixCSR(

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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.

Comment thread lib/mkl/wrappers_sparse.jl Outdated
(:onemklZsparse_set_csr_data_64, :ComplexF64, :Int64))
@eval begin

function oneSparseMatrixCSR(

Copy link
Copy Markdown
Member

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?

Comment thread lib/mkl/interfaces.jl
# 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')

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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?

Comment thread lib/mkl/interfaces.jl

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')

Copy link
Copy Markdown
Member

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'.

Comment thread lib/mkl/interfaces.jl

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')

Copy link
Copy Markdown
Member

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).

@codecov

codecov Bot commented Oct 1, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 32.38636% with 119 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.08%. Comparing base (ff1602e) to head (5c833a7).

Files with missing lines Patch % Lines
lib/mkl/interfaces.jl 32.38% 119 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michel2323 michel2323 changed the title Fix sparse gemm, gemv, and mul Fix sparse gemm, gemv, and mul for complex types Oct 31, 2025
@michel2323

Copy link
Copy Markdown
Member Author

Closing: the core of this landed on main through #602 (CSC multiplication fix, BlasReal → BlasFloat, tA passed through to the wrappers) and #630 (storage-specific mul! methods). The "sparse LinearAlgebra mul" testset on main now checks opa(dA) * dx and opa(dA) * dB for identity/transpose/adjoint over Float32/64 and ComplexF32/64 on CSR and CSC, which is what this PR set out to fix. The explicit Transpose{...}/Adjoint{...} dispatches here are not needed since LinearAlgebra already lowers those wrappers to the tA character.

What is still missing on main is a high-level mul! for oneSparseMatrixCOO; that is small enough to do as a fresh PR against current main rather than rebasing this one.

@michel2323 michel2323 closed this Sep 24, 2026
@amontoison
amontoison deleted the ms/sparse_fix branch September 24, 2026 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants