Cleanup Wrappers and Tests #37

alyst · 2023-05-06T21:55:26Z

This is a subset of #30 excluding the Matrix * SparseMatrix:

~~adds support for MKL 2023.x~~
simplifies the MKLSparse wrappers by introducing mkl_call(funcname, T, args...) generated function that automatically transforms templates like mkl_Tcscmv into appropriate MKLSparse function names for a specific input types (e.g. mkl_dcscmv).
@eval-based generation of all possible wrapper method parameter combinations is replaced by a single method with a proper signature and the use of mkl_call() that automatically generates a proper method MKLSparse method name to call. That should make debugging and code coverage analysis easier.
some unnecessary "type piracy" of Base.* was removed, I've also added comments explaining the motivation for specific LinearAlgrebra/Base (re)definitions
fixes 4-arg ldiv!() from ldiv!(a, A, B, C) to ldiv!(C, A, B, a) to be compatible with 3-arg ldiv!() already in Base (AFAIK 4-arg is only defined in MKLSparse). This would break any packages/script that use MKLSparse.jl 4-arg ldiv!(), but this should be a rather exotic case.
streamlines testing, so that all combination of sparse matrix value, index types, storage as well as triangular, symmetric, adjoint and transposed 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
in tests maximum(abs.(...)) <= eps is replaced with ≈ atol=...
update CI.yml to use julia-actions
enable code coverage

No new version of MKLSparse.jl was tagged after #31 got merged. It could be done before/after this PR.

alyst · 2023-05-07T01:26:26Z

@KristofferC Considering sparse*matrix failures on Julia nightly failures on macOS. The tests in this PR fails just as in #30.
It's interesting, though, when I am switching from Sparse BLAS API (see test results) to inspector/executor API, the amount of failures is 2x reduced. Also, when given the same input data and using the same SparseBLAS routine under the hood, the mul!(C, A, B, a, 0), mul!(C, A, B) and A * B alternatives fail or succeed randomly, and changes from run to run. Could it be something related to e.g. array memory management on Julia-nightly at macOS?

codecov-commenter · 2023-05-07T04:46:38Z

Codecov Report

❗ No coverage uploaded for pull request base (master@925146a). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #37   +/-   ##
=========================================
  Coverage          ?   10.41%           
=========================================
  Files             ?        6           
  Lines             ?     1258           
  Branches          ?        0           
=========================================
  Hits              ?      131           
  Misses            ?     1127           
  Partials          ?        0

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alyst · 2023-06-18T02:45:25Z

Either Julia nightly was fixed on macOS or with the given explicitly specified random seed the multiplication error does not manifest itself. I think the PR is ready for review.

cc @KristofferC

amontoison · 2023-06-19T02:14:42Z

@alyst I should be able to review the PR next week.

rayegun · 2023-06-19T04:03:25Z

src/matmul.jl

-                        return cscsm!($tchar, $T(α), matdescra(A), _get_data(A), B, C)
-                    end
-                end
+                LinearAlgebra.ldiv!(C::StridedVector{$T}, A::$AT, B::StridedVector{$T}, α::Number) =


Could we document this? It matches mul! so it's not entirely unexpected

You mean add the docstring?

alyst · 2023-09-27T20:22:36Z

bump. I would really appreciate if someone can review the PR :)

ViralBShah · 2024-05-14T14:20:41Z

@alyst Long pending here, but seems like we should get this merged if you still have the bandwidth for it.

amontoison · 2024-07-07T22:45:38Z

@alyst I have switched to the new API and generated all wrappers in src/libmklsparse.jl.
This should simplify your PR.

and some whitespace cleanups

@test

so that the tests support standard @test kwargs and logic, as well as type inference for BLAS calls

to be compatible with 3-arg ldiv!() in base Julia; add tests for 3- and 4-arg ldiv!()

replace implicit counter increment with method call (so that the behaviour could be changed globally)

the default ones seem to not use the 5-arg mul!() defined in MKLSparse.jl

for keeping github-actions up-to-date

in a hope to increase the chances of compile-time evaluation and constant propagation

alyst · 2024-08-23T21:24:20Z

@amontoison Thank you! I've rebased this PR on top of the current master.
In comparison to the previous revision, I went further with the generated functions approach and completely replaced @eval-based wrappers generation (both for intermediate wrappers like mv!() and LinearAlgebra.jl overloads).
That should make it easier for debugging and coverage reports.

I've checked, and on 1.10.4 the @code_llvm mv!(...) output suggests that the generated function mkl_call() gets inlined (and so the exact name of the MKLSparse.lib function to call gets resolved at compile time).
So there should be no performance penalty for using the generated functions (at least for the repeated calls).

I've also updated the CI to use Julia actions. One thing that does not work is the upload of the coverage reports to the codecov: it needs a token, and I don't have access to modifying this repo secrets.

Would be nice if somebody reviews the PR before it becomes stale again.
Once it's merged, I'll rebase #30 so that MKLSparse.jl supports more combinations of input parameters for sparse linalg routines.

amontoison · 2024-08-23T21:46:57Z

Thanks @alyst !!
I will update the token for coverage reports.

alyst marked this pull request as draft May 6, 2023 22:06

alyst force-pushed the cleanup_wrappers_and_tests branch 2 times, most recently from d8bce50 to a2b6e6b Compare May 6, 2023 23:24

alyst force-pushed the cleanup_wrappers_and_tests branch 3 times, most recently from 774508b to d08f03c Compare June 17, 2023 23:58

alyst marked this pull request as ready for review June 17, 2023 23:59

alyst force-pushed the cleanup_wrappers_and_tests branch 2 times, most recently from 83b5cee to f01272c Compare June 18, 2023 02:37

alyst requested review from amontoison and KristofferC June 18, 2023 05:33

rayegun reviewed Jun 19, 2023

View reviewed changes

alyst mentioned this pull request Sep 27, 2023

Fix Backend Compatibility JuliaDiff/DiffTests.jl#17

Open

alyst and others added 10 commits August 22, 2024 08:54

mkl_size(mtx, dim)

c3cb598

and some whitespace cleanups

reduce code duplication

4e2c88a

add convert(matrix_descr, matdescr)

9285ab8

cleanup&generalize matrices size checks

a3aaeee

replace @test_blas with @test ...@blas()...

fdc76a9

so that the tests support standard @test kwargs and logic, as well as type inference for BLAS calls

tests: use the standard approx atol=... approach

68c730a

tests: refactor, test all BLAS eltypes

eae49ea

tests: streamline testing special matrix classes

d5abad7

test sparse-to-dense conversion

c49a927

fix matrixdescr of a general spmatrix

7d6a545

alyst and others added 4 commits August 23, 2024 00:36

add convert(matrix_descr, str) tests

8fad618

tests: explicit random seed

9a24dc9

fix 4-arg ldiv!() declaration

e46f3f3

to be compatible with 3-arg ldiv!() in base Julia; add tests for 3- and 4-arg ldiv!()

improve SPBLAS call counting

65312d6

replace implicit counter increment with method call (so that the behaviour could be changed globally)

alyst force-pushed the cleanup_wrappers_and_tests branch 2 times, most recently from f235e88 to 01e873e Compare August 23, 2024 08:01

alyst marked this pull request as draft August 23, 2024 08:07

alyst force-pushed the cleanup_wrappers_and_tests branch 2 times, most recently from 3765196 to 069dfc4 Compare August 23, 2024 09:18

alyst marked this pull request as ready for review August 23, 2024 18:11

alyst and others added 6 commits August 23, 2024 14:11

use mkl_call() for eval-less MKL method wrapping

09ecec1

add 3-arg mul!() overloads

b00972a

the default ones seem to not use the 5-arg mul!() defined in MKLSparse.jl

tests: support CSR and COO sparse matrices

3d50abc

add dependabot.yml

55a6d41

for keeping github-actions up-to-date

ci: use julia-actions

cb8cc55

inline annotations for convert() methods

260de15

in a hope to increase the chances of compile-time evaluation and constant propagation

alyst force-pushed the cleanup_wrappers_and_tests branch from 069dfc4 to 260de15 Compare August 23, 2024 21:13

alyst assigned alyst and unassigned alyst Aug 23, 2024

amontoison approved these changes Aug 23, 2024

View reviewed changes

amontoison merged commit 6a63e07 into master Aug 23, 2024
8 checks passed

amontoison deleted the cleanup_wrappers_and_tests branch August 23, 2024 21:46

alyst mentioned this pull request Aug 25, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Wrappers and Tests #37

Cleanup Wrappers and Tests #37

alyst commented May 6, 2023 •

edited

Loading

alyst commented May 7, 2023 •

edited

Loading

codecov-commenter commented May 7, 2023 •

edited by codecov bot

Loading

alyst commented Jun 18, 2023

amontoison commented Jun 19, 2023

rayegun Jun 19, 2023

alyst Sep 27, 2023

alyst commented Sep 27, 2023

ViralBShah commented May 14, 2024

amontoison commented Jul 7, 2024

alyst commented Aug 23, 2024

amontoison commented Aug 23, 2024

Cleanup Wrappers and Tests #37

Cleanup Wrappers and Tests #37

Conversation

alyst commented May 6, 2023 • edited Loading

alyst commented May 7, 2023 • edited Loading

codecov-commenter commented May 7, 2023 • edited by codecov bot Loading

Codecov Report

alyst commented Jun 18, 2023

amontoison commented Jun 19, 2023

rayegun Jun 19, 2023

Choose a reason for hiding this comment

alyst Sep 27, 2023

Choose a reason for hiding this comment

alyst commented Sep 27, 2023

ViralBShah commented May 14, 2024

amontoison commented Jul 7, 2024

alyst commented Aug 23, 2024

amontoison commented Aug 23, 2024

alyst commented May 6, 2023 •

edited

Loading

alyst commented May 7, 2023 •

edited

Loading

codecov-commenter commented May 7, 2023 •

edited by codecov bot

Loading