-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 compatibility issues with SciPy 1.14 #13358
base: main
Are you sure you want to change the base?
Conversation
The main change here is that SciPy 1.14 on macOS now uses the system Accelerate rather than a bundled OpenBLAS by default. These have different characteristics for several LAPACK drivers, which caused numerical instability in our test suite. Fundamentally, these problems existed before; it was always possible to switch out the BLAS/LAPACK implementation that SciPy used, but in practice, the vast majority of users (and our CI) use the system defaults. The modification to `Operator.power` to shift the branch cut was suggested by Lev. As a side-effect of how it's implemented, it fixes an issue with `Operator.power` on non-unitary matrices, which Sasha had been looking at. The modification to the Choi-to-Kraus conversion reverts back from a Schur-based decomposition to an `eigh`-based one. This was noted in a code comment that it was causing instability, and tracing the git history through to fdd5603 (Qiskitgh-3884) suggests that this was around the time of Scipy 1.1 to 1.3, with OpenBLASes around 0.3.6. The bundled version of OpenBLAS with SciPy 1.13 was 0.3.27, so several releases on. If `eigh` problems re-appear, we can look at changing the decomposition back to something else, but the current `eigh` seems to be more stable than the Schur form for a wider range of inputs now. Co-authored-by: Lev S. Bishop <[email protected]> Co-authored-by: Alexander Ivrii <[email protected]>
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11465405675Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to explicitly test eigenvalues barely either side of the intended branch cut, but perhaps that would be a pain to write?
I could adapt the exact same test, I think? We'd just need to take care not to couple the test to the particular choice of the rotation epsilon. |
Yeah, it might get confusing though with the different epsilons involved. There's eps_bc that defines the intended position of the branch cut, eps_fp thats potential numerical imprecision during the test, and then eps_test that's how close the test comes either side of the branch cut. I guess you want eps_test > eps_fp and then check eigenvalues at eps_bc±eps_test away from the axis. |
I was hoping to make |
That seems in line with how we deal with other tolerance-ish things. I don't have a strong opinion. |
Seems like |
Summary
The main change here is that SciPy 1.14 on macOS now uses the system Accelerate rather than a bundled OpenBLAS by default. These have different characteristics for several LAPACK drivers, which caused numerical instability in our test suite. Fundamentally, these problems existed before; it was always possible to switch out the BLAS/LAPACK implementation that SciPy used, but in practice, the vast majority of users (and our CI) use the system defaults.
The modification to
Operator.power
to shift the branch cut was suggested by Lev. As a side-effect of how it's implemented, it fixes an issue withOperator.power
on non-unitary matrices, which Sasha had been looking at.The modification to the Choi-to-Kraus conversion reverts back from a Schur-based decomposition to an
eigh
-based one. This was noted in a code comment that it was causing instability, and tracing the git history through to fdd5603 (gh-3884) suggests that this was around the time of Scipy 1.1 to 1.3, with OpenBLASes around 0.3.6. The bundled version of OpenBLAS with SciPy 1.13 was 0.3.27, so several releases on. Ifeigh
problems re-appear, we can look at changing the decomposition back to something else, but the currenteigh
seems to be more stable than the Schur form for a wider range of inputs now.Details and comments
Let's see what happens in the full test matrix, especially Windows (which iirc is usually the source of
eigh
problems). It worked with both SciPy 1.13 and 1.14 on my Intel Mac.Fix #12655
Fix #13305
Fix #13307
This should hopefully unblock #13309.
This obsoletes the bugfix of #13319 as a side-effect of solving #13305, but the additional method / keyword argument to
power
there is still likely very useful (though will need adapting to use the same technique as here).