ENH: linalg: return complex arrays from eigvals/eigvecs#30411
ENH: linalg: return complex arrays from eigvals/eigvecs#30411mattip merged 5 commits intonumpy:mainfrom
Conversation
|
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-image 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). Details |
|
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 :) |
|
Reformatted the release note |
|
Updated to maintain strict backwards compatibility of root-finding routines. The reasoning is:
I tested locally that scipy and scikit-learn are not affected by this change (scikit-learn has some tests that rely on numpy's The scikit-image fix is scikit-image/scikit-image#8054 Apologies @mattip --- I rebased/force-pushed without checking your edits for the release note; tried recreating your edits manually. Types: @jorenham I've to admit I'm lost in the sea of overloads. What is the difference between |
unlike |
|
Thanks @ev-br |
Let's keep an eye out for breaking issues |
NumPy plans to change it's `linalg.eig` routine to always return complex results. Currently, it returns either complex or real values, depending on whether the eigenvalues lie on the real axis or not. See numpy/numpy#30411 The only effect on scikit-image AFAICS, is in the ellipse estimation routines, which require that eigenvalues are real. In fact, the story seems to be somewhat convoluted: - some datasets produce non-zero imaginary parts, which break `EllipseModel.fit`, #7013 - the current failure mode is a TypeError from an in-place modulo operation, `phi %= np.pi`, where `phi` is constructed from eigenvalues/eigenvectors - #7013 (comment) asks for a reproducer, and numpy/numpy#29000 (comment) has some discussion of potential fixes/enhancements This PR makes a minimal fix to make `EllipseModel.fit`: take the real part of eigenvalues/eigenvectors explicitly, and raise an error if either of them has non-zero imaginary parts. --------- Co-authored-by: Stefan van der Walt <stefanv@berkeley.edu>
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.