Skip to content
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

More sparse operations support #50

Merged
merged 28 commits into from
Jan 3, 2025
Merged

More sparse operations support #50

merged 28 commits into from
Jan 3, 2025

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Sep 10, 2024

This PR adds support for more SparseMKL operations:

  • dense := sparse * sparse (sp2md!())
  • sparse := sparse * sparse (including the in-place one that can optionally disable checks for the sparsity structure) (sp2m(), sp2m!())
  • dense := X * A * X^T (syprd!())
  • dense := X * X^T (syrk!())

Implementation Details

  • while Intel MKL sp2m() supports
    reusing the sparsity structure in principle, it only supports the sparsity structure allocated by Intel MKL.
    It is therefore not possible to directly update the existing SparseMatrixCSC object, reusing the colptr, rowval and updating the nzval only.
    Instead, each sp2m!() call has to recalculate the sparsity structure, so it is not efficient as it could have been.
  • Intel MKL syrkd() and
    syprd() routines only support the CSR format and only fill the upper triangular part of the matrix.
    So, while MKLSparse.jl wrappers (syrkd!() and syprd!()) implement CSC support by treating it as a transpose/adjoint of the CSR matrix, it
    does not work for complex numbers and operations like C := a * A^H A + b * C, because transposition trick messes with which half of the hermitian C
    is used. So complex support for syrkd!() and syprd!() is disabled
  • Because of these complications, syrkd!() and syprd!() were not "plugged" into mul!() calls like low rank dense BLAS counterparts.
    Also, copytri!() is actually very expensive for large matrices due to cache misses, so doing it by default may result in degraded performance.
  • For syprd!() there is also no direct LinearAlgebra call to overload.
    Potentially, MKLSparse.jl may support PDMats.jl via extension mechanism to implement X_A_Xt!() via syprd!().
  • syrk() only returns the upper triangular part of the resulting sparse matrix. Since allocating and computing the lower triangular part in that case could be even more expensive than
    for the dense case, syrk(..., copytri=true) is not allowed.
  • spmm() and spmmd!() wrappers were added, but as they are just more limited versions of sp2m!() and sp2md!(), they were not tested

@amontoison
Copy link
Member

@alyst Can you split the PR to add what is already working?

@alyst
Copy link
Contributor Author

alyst commented Sep 17, 2024

@amontoison I will rebase it on top of the 2 PRs I have submitted today. I think all of the new operations should be working (I use them in my code), but I would need to fix/add the tests.

@amontoison
Copy link
Member

Perfect, thanks @alyst 👍
What we should also try to interface in the future is the sparse preconditioner.

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 80.44280% with 53 lines in your changes missing coverage. Please review.

Project coverage is 44.39%. Comparing base (cb0852e) to head (4a7c463).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/generic.jl 79.50% 25 Missing ⚠️
src/types.jl 40.90% 13 Missing ⚠️
src/interface.jl 80.85% 9 Missing ⚠️
src/mklsparsematrix.jl 93.65% 4 Missing ⚠️
src/utils.jl 88.23% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #50       +/-   ##
===========================================
+ Coverage   32.98%   44.39%   +11.40%     
===========================================
  Files           7        8        +1     
  Lines        1258     1489      +231     
===========================================
+ Hits          415      661      +246     
+ Misses        843      828       -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alyst alyst marked this pull request as ready for review January 3, 2025 20:24
@alyst
Copy link
Contributor Author

alyst commented Jan 3, 2025

@amontoison Finally I was able to add the tests for this PR, so it should be ready for review. I've also added "Implementation Notes" section to the PR description to provide some context about what was implemented and what are the limitations.

@amontoison
Copy link
Member

@alyst I will review that :)
Do you need the release v2025.0.0 of MKL_jll.jl?

@amontoison amontoison merged commit 18ad424 into master Jan 3, 2025
10 checks passed
@amontoison amontoison deleted the more_ops branch January 3, 2025 22:10
@alyst
Copy link
Contributor Author

alyst commented Jan 3, 2025

I will review that :) Do you need the release v2025.0.0 of MKL_jll.jl?

Thank you! I think v2025.0 is not required, any v2024 should work as well (v2025 is subject to the same restrictions I've mentioned in the implementation notes).

@amontoison
Copy link
Member

LGTM, I merged and tagged a new release 👍

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