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

Fixed #38346: Eigen decomposition of Symmetric Matrix containing NaNs now throws exception #38408

Merged
merged 7 commits into from
Dec 9, 2020

Conversation

kc611
Copy link
Contributor

@kc611 kc611 commented Nov 12, 2020

Fixes JuliaLang/LinearAlgebra.jl#780.

As suggested by @KlausC in the issue

There seems missing a call to ckfinite or analogous in syevr!

adding a call to 'ckfinite' in 'syevr' is a nice fix for the problem. Since Eigen decomposition for both symmetric and hermitian matrix is making call to 'syevr' function it fixes the issue for both. Diagonal matrices already had such a fail-safe for NaN values

if any(!isfinite, D.diag)
throw(ArgumentError("matrix contains Infs or NaNs"))
end

@dkarrasch
Copy link
Member

@kc611 Thank you and welcome! Can you please add a test (à la @test_throws ...) to make sure the function throws when it should throw?

@dkarrasch dkarrasch added linear algebra Linear algebra needs tests Unit tests are required for this change labels Nov 12, 2020
@kc611
Copy link
Contributor Author

kc611 commented Nov 12, 2020

It seems that there was already a test present for Eigen of matrices with NaN values. I extended it's functionality to Symmetric, Hermitian and Diagonal(just in case) ones.

dkarrasch
dkarrasch previously approved these changes Nov 12, 2020
stdlib/LinearAlgebra/src/lapack.jl Outdated Show resolved Hide resolved
Copy link
Member

@JeffBezanson JeffBezanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has comments that need to be addressed.

@dkarrasch dkarrasch dismissed their stale review November 26, 2020 11:11

Missing triangular check

@kc611
Copy link
Contributor Author

kc611 commented Nov 27, 2020

Changed the iterations to column-wise (the loops looks a bit clean and consistent that way) and added one based indexing check and @inbounds as suggested.

Also I was thinking about adding a test case where the code is not supposed to throw exception. But dropped the idea since I could not find any appropriate way for doing that (#18780). Any suggestions?

@KlausC
Copy link
Contributor

KlausC commented Nov 27, 2020

To verify, that a test does not throw and exception you could do for example:

@test eigen(...) isa Eigen
or
@test something....   !== []
or
@test something...  !== nothing  (if you expect any return type != nothing

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dkarrasch dkarrasch added the merge me PR is reviewed. Merge when all tests are passing label Dec 9, 2020
@dkarrasch dkarrasch closed this Dec 9, 2020
@dkarrasch dkarrasch reopened this Dec 9, 2020
@dkarrasch dkarrasch added backport 1.6 Change should be backported to release-1.6 and removed merge me PR is reviewed. Merge when all tests are passing needs tests Unit tests are required for this change labels Dec 9, 2020
@dkarrasch
Copy link
Member

The usual unrelated test failures. Thank you very much @kc611, and sorry that it took a while. Your contribution is much appreciated!

@dkarrasch dkarrasch merged commit a813a6e into JuliaLang:master Dec 9, 2020
KristofferC pushed a commit that referenced this pull request Dec 14, 2020
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Dec 19, 2020
staticfloat pushed a commit that referenced this pull request Jan 15, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LinearAlgebra: eigen decomposition of Symmetric Matrix containing NaNs never throws. It just runs forever.
5 participants