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

Exposing single precision support from MKL #108

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mipals
Copy link
Member

@mipals mipals commented Feb 5, 2025

 - Adding check for square matrix
 - Changing Project.lic to Panua.lic and name of of file form "project_pardiso.jl" to "panua_pardiso.jl"
@mipals
Copy link
Member Author

mipals commented Feb 5, 2025

Tests fail on 1.6 as we compare the against results from SparseArrays which did not support Float32 and ComplexF32 on 1.6. We could either stop testing on 1.6 and move the new LTS of 1.10 or we could compare with an answer computed using Float64 and ComplexF64. What do you think @j-fu ?

@j-fu
Copy link
Collaborator

j-fu commented Feb 6, 2025

I think if we drop testing on 1.6 we also would have to bump the min julia version, most of the ecosystem including LinearSolve.jl is going to 1.10 anyway.
Just had a look, MKL.jl now requires 1.9. Which suggest that we would test on 1.6 with a quite old version of MKL.

OTOH this would be kind of a pity as there is no real reason to in the package code which would require a higher version of 1.6. But I think this is too much sentiment, and we should focus on keeping maintenance simple...

Keeping tests running for 1.6 by always computing the reference solution using Float64/CompelxF64.
Converting results back to Float32/ComplexF32.
@KristofferC
Copy link
Member

But I think this is too much sentiment, and we should focus on keeping maintenance simple...

Agree with this.

Regarding this PR, should there be a check that the iparam is set correctly similar to the checks in

Pardiso.jl/src/Pardiso.jl

Lines 326 to 329 in a645c66

if Tv <: Complex && isreal(get_matrixtype(ps))
throw(ErrorException(string("input matrix is complex while PardisoSolver ",
"has a real matrix type set: $(get_matrixtype(ps))")))
end

?

@mipals
Copy link
Member Author

mipals commented Feb 6, 2025

FWIW the package would still be compatible with older version - it would just be the MKLPardiso tests for Float32/ComplexF32 that would fail. But I agree that it would probably be best for the tests to still run on 1.6.

Because of this I've fixed the tests so that they should not fail on 1.6 by computing the reference solution in Float64/ComplexF64 and then converting the results back to Float32/ComplexF32. For some reason a few of the tests for Float32 inputs fail when using MKL_jll v. 2025 (locally the tests seem to run fine using e.g. 2023, 2024 but fail similar to the CI when using 2025).

…sting that the solution actually works rather than comparing with SparseArrays
@mipals
Copy link
Member Author

mipals commented Feb 7, 2025

The error was not in the MKL_jll version, but rather that the hermitian positive tests where quite ill-conditioned (I still don't understand why this was only caught on 1.6 - did something change with Random since then?).

The current solution is to add a diagonal to the hermitian positive tests. At the same time i've changed the tests so that they check that solution works rather than comparing with the answer from SparseArrays (in the failures yesterday both solutions were actually wrong but the tests failed only because they two solutions where not equal).

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.

3 participants