ENH: linalg: return complex arrays from eigvals/eigvecs #30411
+30
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Here is a quick session of downstream testing: I ran test suites for SciPy, scikit-learn, scikit-image, networkx and pandas. Short version: scipy has small issues which I'll take care of; scikit-learn has a small issue which I'm volunteering to fix to restore the status quo; scikit-learn and networkx are not affected; pandas is most likely not affected. Testing notes are under the fold. I'm happy to both run tests for other downstream projects (which ones?), and send downstream patches to restore the status quo after this PR lands (assuming that it is, of course). |
Member
|
Coincidentally I was just working on the numpy/numpy/linalg/_linalg.pyi Lines 287 to 299 in a64d310 So yea I like this :) And on that note, it would help if you could also update the stubs accordingly now :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A (simplest) companion to #29000 : change the dtype of
eig/eigvalsreturns from maybe float if starts align, else complex to always complex.As an opinionated pitch from the long discussion in gh-29000:
Hence this PR proposes to stop going out our way, and return what the math says: complex arrays.
This is a breaking change
What is the user impact?
Code search shows two kinds of affected usage:
This is typically a sign that the user code genuinely misses a case where the eigenvalues are not on a real line.
Example: scikit-image/scikit-image#7013 (comment)
Note that this scikit-image issue is that the scikit-image code assumes real eigenvalues and fails where they are genuinely not.
The downstream fix could be along the lines of adding in the user code
to bring the behavior to what it is today. If wanted, they can add a warning asking a user to report the reproducer, as the scikit-learn issue asks for.
eigvals(covariance_matrix).For a covariance matrix, we know that it's positive definite and the eigenvalues actually are real-values. Hence the fix is to use
eighinstead.I think the best we can do is to add a note to this effect.
An opinionated summary of alternatives discussed in gh-29000:
eigemit a warning of impending change.The warning is extraneous and annoying for those users who are not affected or have already adapted.
I frankly don't think it's very useful to users. Large, well-maintained users can just make the change. The long tail of incorrect usage is unlikely to be able to adapt twice (add the keyword, adapt to the change, wait for a couple of years, remove the keyword)
eigandeigvals.Feels like a lot of churn across the ecosystem.