Use evd driver for eigh in TwoQubitWeylDecomposition #5251
Conversation
The hermitian optimized eigen solver function in scipy has shown to possibly produce varied results in different environments. For example, the matrix returned by the eigh() call in the TwoQubitWeylDecomposition class's __init__ method when called for a CXGate: TwoQubitWeylDecomposition(Operator(CXGate())) was observed to be [[ 0.70710678 0. -0.70710678 0. ] [ 0. -0.70710678 0. 0.70710678] [ 0. 0.70710678 0. 0.70710678] [ 0.70710678 0. 0.70710678 0. ]] on one system/environment but [[ 0.70710678 0. -0.70710678 0. ] [ 0. 0.70710678 0. 0.70710678] [ 0. -0.70710678 0. 0.70710678] [ 0.70710678 0. 0.70710678 0. ]] on a different environment. This difference was resulting in inconsistent decompositions being generated and made reproducing results from the transpiler difficult. In testing the numpy equivalent function [2] has proven to be more reliable across different environments. This commit switches the eigh() usage in the TwoQubitWeylDecomposition class to use the numpy function instead this should fix the inconsistent decompositions we were seeing between environments. [1] https://docs.scipy.org/doc/scipy/reference/generated/scipy.linalg.eigh.html [2] https://numpy.org/doc/stable/reference/generated/numpy.linalg.eigh.html
This commit removes a test Qiskit#4835 which was added to validate that the internal values for the decomposer didn't sign flip in the specific case that the PR was fixing. However with the use of the numpy eigh() function these internal values no longer the same. So instead of updating the test this just deletes it since the original case it was catching is no longer applicable.
|
I just remembered that we will need to bump the minimum version of numpy in the requirements list to 1.18.0 with this. The numpy eigh() function was added in 1.18.0 |
|
So they both use zheevd in general. So it seems hard to fathom that there is any difference between the two when using that driver. Scipy selects between multiple drivers, but it can also be manually set. Did you try setting the driver explicitly in the scipy version? |
|
I didn't mostly because I assumed the driver selection would be deterministic and it would be using the same driver on both systems I was testing on (@ajavadia also had the same issue on two of his systems). I'll give setting a driver explicitly a try on Monday. But at the end of the day if this solves the issue for us I don't see the harm in switching to use the numpy routine even if why it's happening is a bit of a mystery. My first thought after figuring out that |
|
Actually looking theough the docs it is ev by default in scipy and evd in numpy. So driver='evd' in scipy should return the same in both modules |
|
And yes not the end of the world, but then we wouldn't need to bump up a dependency for no reason. |
|
Ok, tested locally on my 2 systems and using evd as the driver produces a consistent result. So I'll update this to just use that with the scipy method. That way we can avoid the requirements bump |
This commit reverts back to using the scipy eigh() function but sets a the lapack driver to be fixed as evd. The numpy routine was using the evd driver while scipy would default to use ev as the driver. Switching the scipy driver from ev to evd seems to be the key difference between the routines which was causing the inconsistent results with scipy. Approaching it this way means we avoid need to bump the minimum numpy version in the requirements list since the numpy eigh() function was added in a fairly recent version.
mtreinish
changed the title
|
@ajavadia do you want me to remove that rounding in this PR or do it in a followup? |
|
follow up is fine, thanks |
In Qiskit#5251 we updated the LAPACK driver used for the scipy hermitian optimized eigensovler to use 'evd' instead of the default 'ev'. However, this implicitly added a scipy minimum version of 1.5.0 because the driver kwarg was only added to the eigh() function in that release. [1] In that PR we didn't bump the minimum scipy version to reflect this. However, scipy 1.5.0 has a version conflict with our downstream users, mainly qiskit-aqua/qiskit-chemistry which use PySCF as an optional dependency. PySCF has a capped version of scipy in their requirements [2] which makes bumping our minimum scipy version intractable currently. To avoid this issue, this commit switches to use numpy's eigh() function [3] instead of scipy's. The numpy function uses the evd driver and was what we originally used in Qiskit#5251 but was switched to use scipy's eigh() with the driver kwarg to avoid bumping the minimum version of numpy. However, between bumping the minimum numpy version and bumping the minimum scipy version, the numpy version is less problematic. [1] https://docs.scipy.org/doc/scipy/reference/release.1.5.0.html#scipy-linalg-improvements [2] https://github.com/pyscf/pyscf/blob/v1.7.5/setup.py#L490 [3] https://numpy.org/doc/stable/reference/generated/numpy.linalg.eigh.html
|
Well it turns out that the https://docs.scipy.org/doc/scipy/reference/release.1.5.0.html#scipy-linalg-improvements So we should have bumped the minimum version in this PR. But, it turns out chemistry in aqua doesn't work with scipy 1.5 because pyscf caps at <1.5. I opened to #5263 address this an unbreak aqua ci. |
* Use numpy eigh instead of scipy eigh with driver kwarg In #5251 we updated the LAPACK driver used for the scipy hermitian optimized eigensovler to use 'evd' instead of the default 'ev'. However, this implicitly added a scipy minimum version of 1.5.0 because the driver kwarg was only added to the eigh() function in that release. [1] In that PR we didn't bump the minimum scipy version to reflect this. However, scipy 1.5.0 has a version conflict with our downstream users, mainly qiskit-aqua/qiskit-chemistry which use PySCF as an optional dependency. PySCF has a capped version of scipy in their requirements [2] which makes bumping our minimum scipy version intractable currently. To avoid this issue, this commit switches to use numpy's eigh() function [3] instead of scipy's. The numpy function uses the evd driver and was what we originally used in #5251 but was switched to use scipy's eigh() with the driver kwarg to avoid bumping the minimum version of numpy. However, between bumping the minimum numpy version and bumping the minimum scipy version, the numpy version is less problematic. [1] https://docs.scipy.org/doc/scipy/reference/release.1.5.0.html#scipy-linalg-improvements [2] https://github.com/pyscf/pyscf/blob/v1.7.5/setup.py#L490 [3] https://numpy.org/doc/stable/reference/generated/numpy.linalg.eigh.html * Remove numpy min version bump * Remove leftover driver kwarg
Summary
The hermitian optimized eigen solver function in scipy has shown
to possibly produce varied results in different environments. For
example, the matrix returned by the
eigh()call in theTwoQubitWeylDecompositionclass's__init__method when called fora CXGate:
was observed to be
on one system/environment but
on a different environment. This difference was resulting in
inconsistent decompositions being generated and made reproducing results
from the transpiler difficult. In testing the numpy equivalent
function [2] has proven to be more reliable across different
environments. It turns out that this was because the LAPACK driver
used by the numpy routine was
evdwhile the default driver for scipywas
ev. Switching the scipy driver to useevdtoo fixes this issue. Thiscommit switches the
eigh()usage in theTwoQubitWeylDecompositionclass to explicilty set the driver to
evd instead of the defaultev`,this should fix the inconsistent decompositions we were seeing between
environments.
Details and comments
[1] https://docs.scipy.org/doc/scipy/reference/generated/scipy.linalg.eigh.html
[2] https://numpy.org/doc/stable/reference/generated/numpy.linalg.eigh.html