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

Vec * SparseMat & Mat * SparseMat support and other improvements #30

Merged
merged 21 commits into from
Aug 26, 2024

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Aug 20, 2022

This PR (rebased on top of #37)

  • simplifies a bit the w1 *rapper generation logic
  • improves describe_and_unwrap() for more adv. cases like LowerTriangular{T, Adjoint{T, SparseMatrixCSC{T}}}, some cleanups to matrix_descr code
  • Char to sparse_index_base_t conversion changed from Z/O to 0/1 as the latter is more straightforward
  • improves A * B overloads -- so the ones that are already routed to MKLSparse overloads of mul!() are not redefined (this is Julia version-specific)
  • streamlines testing, so that all 4 numeric types are covered
  • @test_blas macro replaced by conventional @test macro in combination with @blas macro that allows more fine-grained checks of whether Sparse BLAS MKL methods are being called
  • adds status checks for mkl_sparse_xxx API calls and MKLSparseError exception
  • adds extraction of CSR/CSC matrices from MKLSparse matrix handles (MKLSparseMatrix)
  • last but not the least: the support for Matrix * SparseMatrixCSC multiplication via Sparse BLAS (currently it's being handled by the Base Julia). It uses the fact that ColMajorRes = ColMajorMtx*SparseMatrixCSC is the same as RowMajorRes = SparseMatrixCSR*RowMajorMtx. Unfortunately, the older Sparse BLAS API doesn't support the combination of one-based Sparse matrices and row-major storage, so I had to add some boilerplate code to leverage newer mkl_sparse_xxx API.
  • adds support for vec^H * spM^T and vec^T * spM^H not handled by stdlib -- with the help of internal switching to SparseMatrixCSR

@alyst
Copy link
Contributor Author

alyst commented Aug 20, 2022

I guess the macOS failure (it specifically fails in Matrix*SpMatrix tests) has something to do with MKL_jll version (judging by the file name it is a bit older than Win/Linux ones).
Meanwhile there's Intel MKL 2022.1, perhaps the update would solve the failure.

@alyst
Copy link
Contributor Author

alyst commented Aug 24, 2022

@andreasnoack @KristofferC If you have time, could some of you please take a look at the PR?

@KristofferC
Copy link
Member

I haven't really kept up with the changes in LinearAlgebra so I am not super qualified to look at this. But overall it looks good. I'm happy to make you a collaborator on this repo.

I am a bit confused about the error on macOS nightly though.

I guess the macOS failure (it specifically fails in Matrix*SpMatrix tests) has something to do with MKL_jll version (judging by the file name it is a bit older than Win/Linux ones).

Aren't all builds running with MKL_jll v2022.0.0+0? I think it would be good to look into this a bit more. What difference is it with macOS nightly compared to the other builds?

@alyst
Copy link
Contributor Author

alyst commented Aug 24, 2022

@KristofferC

But overall it looks good. I'm happy to make you a collaborator on this repo.

Thank you!

Aren't all builds running with MKL_jll v2022.0.0+0? I think it would be good to look into this a bit more. What difference is it with macOS nightly compared to the other builds?

Yes, that's right. Also with the recent update to v2022.1 the error doesn't go away.
It is only the specific tests that involve dense_matrix * sparse_matrix multiplication that are failing (on Julia nightly macOS, and for all numeric types), whereas e.g. dense_matrix * sparse_matrix^T does not fail.
In the resulting matrix the first column looks correct, the values in the other cells suggest that during multiplication wrong memory regions are accessed.
I have marked these tests as broken for now.

@alyst
Copy link
Contributor Author

alyst commented Aug 24, 2022

@dmbates Unfortunately, I have discovered your dmb/matrix_sparse branch, which overlaps with this PR, too late. If you have any chance to review, please let me know your suggestions.

@alyst alyst force-pushed the spmatrix_x_matrix branch 2 times, most recently from 8f1c5fa to 5b0559a Compare May 4, 2023 23:21
@KristofferC
Copy link
Member

Did you figure out what caused the previous mac error (considering CI is now green)?

@alyst
Copy link
Contributor Author

alyst commented May 5, 2023

@KristofferC Oh no, unfortunately, it's still there for the new dense_matrix * sparse_matrix on macOS/Julia-nightly as before (nightly on the other OS is fine); updating to MKL 2023.1 doesn't fix it. I've just declared these test as broken.

@alyst alyst mentioned this pull request May 6, 2023
@ViralBShah
Copy link
Member

Mac is just officially unsupported now... @alyst Happy to give you commit access to upgrade this package (sorry I just noticed this now since I usually didn't pay attention to MKLSparse.jl).

@alyst alyst marked this pull request as draft August 24, 2024 20:13
Copy link

codecov bot commented Aug 25, 2024

Codecov Report

Attention: Patch coverage is 93.86503% with 10 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@6a63e07). Learn more about missing BASE report.

Files Patch % Lines
src/types.jl 73.91% 6 Missing ⚠️
src/mklsparsematrix.jl 95.29% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master      #30   +/-   ##
=========================================
  Coverage          ?   24.86%           
=========================================
  Files             ?        7           
  Lines             ?     1693           
  Branches          ?        0           
=========================================
  Hits              ?      421           
  Misses            ?     1272           
  Partials          ?        0           

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

@alyst alyst changed the title Refactorings & Matrix * Sparse-Matrix support Vec * SparseMat & Mat * SparseMat support and other improvements Aug 25, 2024
@alyst alyst marked this pull request as ready for review August 25, 2024 20:23
@alyst
Copy link
Contributor Author

alyst commented Aug 25, 2024

@amontoison Could you please take a look?
There were some memory access error in MKLSparse on windows x86 in the previous workflow run for SparseMatrixCOO tests (so rather exotic).
I don't know what might have caused that -- it's not in the code that uses mkl_sparse_extract() or sparse matrix layout conversion, which is more likely to cause memory corruption.
Maybe it's some internal MKLSparse error.

@alyst alyst requested a review from amontoison August 25, 2024 21:06
@amontoison
Copy link
Member

@alyst For the modification about:

  • `Char to sparse_index_base_t conversion changed from Z/O to 0/1.

It's what is used by many API, in particular GPU routines like CUDA toolkit, oneAPI or ROCm.
Do they changed the API in oneMKL?

To be the best of my knowledge, Z / O is the most common syntax for zero/one based indexing.

@amontoison
Copy link
Member

amontoison commented Aug 25, 2024

Where is the error for SparseMatrixCOO?

Talking about it, we should do extensions for the packages that define sparse COO and CSR matrices instead a doing a copy of the types here.

@alyst
Copy link
Contributor Author

alyst commented Aug 25, 2024

  • `Char to sparse_index_base_t conversion changed from Z/O to 0/1.

It's what is used by many API, in particular GPU routines like CUDA toolkit, oneAPI or ROCm. Do they changed the API in oneMKL?

To be the best of my knowledge, Z / O is the most common syntax for zero/one based indexing.

I can certainly revert it, if Z/O is a common scheme (I find it a bit unfortunate since O resembles 0).
But so far Z/O is a local "syntactic sugar" in MKLSparse.jl, since oneMKL itself uses

@enum sparse_index_base_t::UInt32 begin
    SPARSE_INDEX_BASE_ZERO = 0
    SPARSE_INDEX_BASE_ONE = 1
end

which Z/O map to internally in MKLSparse.jl.

@alyst
Copy link
Contributor Author

alyst commented Aug 25, 2024

Where is the error for SparseMatrixCOO?

The previous CI run: https://github.com/JuliaSparse/MKLSparse.jl/actions/runs/10549759585/job/29225173875#step:6:218

Talking about it, we should do extensions for the packages that define sparse COO and CSR matrices instead a doing a copy of the types here.

Full support for COO & CSR is definitely not my goal. I've just added the bare minimum to enable testing + CSR is needed to implement Mat * SparseMatCSC.
And convert(AbstractSparseMatrix, mtx::MKLSparseMatrix) method from this PR is one step towards enabling spmm, sp2m and sypr support.
If you have a clear vision on how to enable the proper COO/CSR support via extensions (while still providing support for e.g. Mat * SpMatCSC in the main package) -- you are very welcome.

@amontoison amontoison merged commit 14c3b1c into JuliaSparse:master Aug 26, 2024
10 checks passed
@amontoison
Copy link
Member

amontoison commented Aug 26, 2024

LGTM!
We should open some issues for the extensions and random bugs.
I will try to do that tomorrow and update the token for coverage.

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.

4 participants