Fix crash in #3591 #3884
Merged
Fix crash in #3591 #3884
Conversation
This file contains 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
nonhermitian
requested review from
ajavadia and
chriseclectic
as code owners
chriseclectic
approved these changes
Feb 25, 2020
faisaldebouni
pushed a commit
to faisaldebouni/qiskit-terra
that referenced
this pull request
Aug 5, 2020
There is an OpenBlas threading issue affecting scipy.linalg.eigh that is holding up Qiskit#3591. This removes the call to eigh and replaces it with eig that uses a different underlying blas call and just takes the real part of the resulting eigenvalues. This also made explicit two of the tests that were causing issues as they were off by a minus sign in the test suite but were fine when run explicitly. * fix threading crash * fix tests to be explicit
jakelishman
added a commit
to jakelishman/qiskit-terra
that referenced
this pull request
Oct 22, 2024
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]>
jakelishman
added a commit
to jakelishman/qiskit-terra
that referenced
this pull request
Oct 24, 2024
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 test changes to the Kraus and Stinespring modules are to cope with the two operators only being defined up to a global phase, which the test previously did not account for. The conversion to Kraus-operator form happens to work fine with OpenBLAS, but caused global-phase differences on macOS Accelerate. A previous version of this commit attempted to revert the Choi-to-Kraus conversion back to using `eigh` instead of the Schur decomposition, but the `eigh` instabilities noted in fdd5603 (Qiskitgh-3884) (the time of Scipy 1.1 to 1.3, with OpenBLASes around 0.3.6) remain with Scipy 1.13/1.14 and OpenBLAS 0.3.27. Co-authored-by: Lev S. Bishop <[email protected]> Co-authored-by: Alexander Ivrii <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 25, 2024
* Fix compatibility issues with SciPy 1.14 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 test changes to the Kraus and Stinespring modules are to cope with the two operators only being defined up to a global phase, which the test previously did not account for. The conversion to Kraus-operator form happens to work fine with OpenBLAS, but caused global-phase differences on macOS Accelerate. A previous version of this commit attempted to revert the Choi-to-Kraus conversion back to using `eigh` instead of the Schur decomposition, but the `eigh` instabilities noted in fdd5603 (gh-3884) (the time of Scipy 1.1 to 1.3, with OpenBLASes around 0.3.6) remain with Scipy 1.13/1.14 and OpenBLAS 0.3.27. Co-authored-by: Lev S. Bishop <[email protected]> Co-authored-by: Alexander Ivrii <[email protected]> * Expose `branch_cut_rotation` parameter in `Operator.power` The rotation used to stabilise matrix roots has an impact on which matrix is selected as the principal root. Exposing it to users to allow control makes the most sense. --------- Co-authored-by: Lev S. Bishop <[email protected]> Co-authored-by: Alexander Ivrii <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
There is an OpenBlas threading issue affecting
scipy.linalg.eighthat is holding up #3591. This removes the call toeighand replaces it witheigthat uses a different underlying blas call and just takes the real part of the resulting eigenvalues.I also made explicit two of the tests that were causing issues as they were off by a minus sign in the test suite but were fine when run explicitly.
Details and comments