SciML / LinearSolve.jl

LinearSolve.jl: High-Performance Unified Interface for Linear Solvers in Julia. Easily switch between factorization and Krylov methods, add preconditioners, and all in one interface.
https://docs.sciml.ai/LinearSolve/stable/
Other
244 stars 52 forks source link

Fix type promote in case b is complex #399

Closed ma-sadeghi closed 10 months ago

ma-sadeghi commented 10 months ago

Fixes #398

codecov[bot] commented 10 months ago

Codecov Report

Merging #399 (d3bc1ce) into main (1c679c6) will increase coverage by 0.20%. Report is 2 commits behind head on main. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
+ Coverage   69.72%   69.92%   +0.20%     
==========================================
  Files          26       26              
  Lines        1965     1965              
==========================================
+ Hits         1370     1374       +4     
+ Misses        595      591       -4     
Files Coverage Δ
src/common.jl 89.28% <100.00%> (-1.79%) :arrow_down:
src/factorization.jl 75.86% <ø> (+0.43%) :arrow_up:

... and 1 file with indirect coverage changes

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

ChrisRackauckas commented 10 months ago

add a test?

ma-sadeghi commented 10 months ago

add a test?

Sure! Just to clarify, do you want to test the "complex" part or the "tol-type-mismatch"? If the former, I was thinking of adding a complex prob3 to basictests.jl

ChrisRackauckas commented 10 months ago

It would be nice to catch both things, but at least showing the behavior on complex should be done since that's a non-trivial interaction I didn't immediately guess.

ChrisRackauckas commented 10 months ago

Tests are failing

ma-sadeghi commented 10 months ago

All tests are passing except for MKL and SVD. From the error messages, I think the MKL wrapper doesn't support complex systems, but not sure what's going on with SVD.

ChrisRackauckas commented 10 months ago

Yes the MKL_jll direct wrapper doesn't support complex, so mark that as broken.

SVD, mark as broken and open a Base Julialang issue.

ChrisRackauckas commented 10 months ago

oh actually that seems like an issue with the cache construction, so ArrayInterface should get some updates and tests for SVD initialization on complex

ChrisRackauckas commented 10 months ago

https://github.com/JuliaArrays/ArrayInterface.jl/blob/master/src/ArrayInterface.jl#L669

The SVD instance creation is only based on A, so the arrays are all complex typed. Should we just always convert b? I think that in order to hit the fast LAPACK functions it's required that b is complex as well and converts internally?

ma-sadeghi commented 10 months ago

Thanks for the fixes. Quick question: Should we fix formatting here or in a separate PR (formatter complains about ext/LinearSolveEnzymeExt.jl, ext/LinearSolveIterativeSolversExt.jl, src/appleaccelerate.jl, src/default.jl, src/mkl.jl, test/basictests.jl, test/default_algs.jl, test/enzyme.jl, test/resolve.jl).

ChrisRackauckas commented 10 months ago

in a separate PR. Right now we're checking if the new format is sufficient and will format all of SciML at the same time.