Skip to content

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Mar 6, 2024

So far nobody managed to reproduce it, and gh-11577 appears to have been fixed upstream. Thus add a test to make sure we catch it if it shows up again.

closes gh-11577

@ev-br ev-br requested review from larsoner and ilayn as code owners March 6, 2024 17:08
@github-actions github-actions bot added the maintenance Items related to regular maintenance tasks label Mar 6, 2024
@tylerjereddy tylerjereddy added this to the 1.13.0 milestone Mar 6, 2024
# isclose chokes on inf/nan values
sup.filter(RuntimeWarning, "invalid value encountered in multiply")
assert np.isclose(D, 4.0, atol=1e-14).any()
assert np.isclose(D, 8.0, atol=1e-14).any()
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use the built-in testing assertion? This works for me locally at least:

--- a/scipy/linalg/tests/test_decomp.py
+++ b/scipy/linalg/tests/test_decomp.py
@@ -369,11 +369,7 @@ class TestEig:
         # The problem is ill-conditioned, and two other eigenvalues
         # depend on ATLAS/OpenBLAS version, compiler version etc
         # see gh-11577 for discussion
-        with np.testing.suppress_warnings() as sup:
-            # isclose chokes on inf/nan values
-            sup.filter(RuntimeWarning, "invalid value encountered in multiply")
-            assert np.isclose(D, 4.0, atol=1e-14).any()
-            assert np.isclose(D, 8.0, atol=1e-14).any()
+        assert_allclose(D[:2], [4.0, 8.0], atol=1e-14)

I think that may give slicker error messages if it ever fails alongside graceful non-finite handling.

Apart from that, looks solid.

Copy link
Member Author

Choose a reason for hiding this comment

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

My original reasoning was 1) I don't remember if assert_allclose is gracefully handling nans and infs, and 2) I don't know if the order is guaranteed, whether two "other" values are always in the 3rd and 4th places.

That said, let's declare both concerns to be FUD until proven real. So the PR is updated with your patch, in a separate commit to simplify reverting if it ever comes to that :-).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, here it is: locally the order is [8.0, 4.0, rest] and CI failures are because it is [4.0, 8.0, rest]. Typically I'd expect that I'd need to sort the eigenvalues before comparing them because the ordering from LAPACK is not guaranteed. Here sorting is not an option because of "rest". OK to revert to the previous manual version?

Copy link
Contributor

Choose a reason for hiding this comment

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

locally the order is [8.0, 4.0, rest]

Ok, so even your local order doesn't match my local order from above.

That's annoying. I see eigvals explicitly documents w as:

The eigenvalues, each repeated according to its multiplicity but not in any specific order.

While the function you use here (eig) doesn't actually say that in the docs. Reverting makes sense then I guess. There are other blemishes in the API too, like the left eigenvector not being normalized while the right one is normalized (a fact that wasn't even documented until recently).

Copy link
Member Author

Choose a reason for hiding this comment

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

There are other blemishes in the API too

ISTM the root is that eig tries to be too helpful in too many ways. Cramming two rather different things into one function (regular and generalized eigenvalue problem, for starters). Then it tries to go beyond what LAPACK actually offers (sorting? normalization?). Water under the bridge now anyway.

@tylerjereddy
Copy link
Contributor

self-merging when CI passes after the revision makes to me I think

At the moment, nobody managed to reproduce it, so add a test to make sure
we catch it if it shows up again.

closes scipygh-11577
@ev-br ev-br force-pushed the add_test_gh_11577 branch from 691ed54 to 912ec4d Compare March 7, 2024 18:16
@tylerjereddy tylerjereddy merged commit 5c364a7 into scipy:main Mar 7, 2024
@tylerjereddy
Copy link
Contributor

I double checked just now that the CI failure can't possibly be related--our latest main against latest NumPy main as of a few minutes ago passes full test suite, and you only added a test without changing source of course.

I think we're just seeing some mismatch on nightlies or not having Sebastian's patch in the base branch or whatever, so in it goes, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generalized eigenvalues are sometimes wrong (on some hardware)
2 participants