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

Fix type promote in case b is complex #399

Merged
merged 18 commits into from
Oct 29, 2023

Conversation

ma-sadeghi
Copy link
Contributor

Fixes #398

@codecov
Copy link

codecov bot commented Oct 23, 2023

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%) ⬇️
src/factorization.jl 75.86% <ø> (+0.43%) ⬆️

... and 1 file with indirect coverage changes

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

@ChrisRackauckas
Copy link
Member

add a test?

@ma-sadeghi
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

Tests are failing

test/basictests.jl Outdated Show resolved Hide resolved
test/basictests.jl Outdated Show resolved Hide resolved
@ma-sadeghi
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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?

test/basictests.jl Outdated Show resolved Hide resolved
test/basictests.jl Outdated Show resolved Hide resolved
@ma-sadeghi
Copy link
Contributor Author

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 ChrisRackauckas merged commit ebde14c into SciML:main Oct 29, 2023
14 of 20 checks passed
@ChrisRackauckas
Copy link
Member

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.

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.

Automatic type promotion of b breaks for complex values
2 participants