Skip to content
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 parameter order in VQE, if the ansatz is resized #7479

Merged
merged 6 commits into from
Jan 13, 2022

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Jan 5, 2022

Summary

Fix a bug in VQE where the parameters of the ansatz were
still explicitly ASCII-sorted by their name if the ansatz was resized. That led to a
mismatching order of the optimized values in the optimal_point attribute of the result
object.

This should have been included in #7099, where the sorting was removed, but was missed.

Details and comments

For example, this led to a wrong optimal circuit if a user tried to construct it using VQEResult.optimal_point.

from qiskit.algorithms import VQE
from qiskit.circuit.library import RealAmplitudes
from qiskit.opflow import Z, I
from qiskit.providers.aer import AerSimulator
from qiskit.quantum_info import Statevector

# Hamiltonian that will lead to more than 11 parameters.
# The default ansatz is RealAmplitudes with 4 rotation layers, hence this will
# produce  a 12-parameter ansatz.
hamiltonian = Z ^ Z ^ Z

sim = AerSimulator(method="statevector")
vqe = VQE(quantum_instance=sim)  # don't set the ansatz but let VQE use the default
result = vqe.compute_minimum_eigenvalue(hamiltonian)

ansatz = RealAmplitudes(hamiltonian.num_qubits)
reconstructed_state  = Statevector(ansatz.bind_parameters(result.optimal_point))
print("Reconstructed state from optimal parameters:")
print(reconstructed_state)
print("State from VQE result:")
print(result.eigenstate)

Instead of the same statevectors, this yields

Optimal circuit using optimal values as list
Statevector([ 0.64431664+0.j, -0.2110578 +0.j,  0.38441563+0.j,
             -0.17266969+0.j, -0.26513136+0.j,  0.02009964+0.j,
              0.50885393+0.j, -0.18190522+0.j],
            dims=(2, 2, 2))
[ 2.81202210e-04+0.j  2.24955744e-01+0.j -1.10683555e-01+0.j
  1.51121930e-04+0.j  7.54507043e-01+0.j  6.74747145e-05+0.j
  9.33921914e-05+0.j -6.06517165e-01+0.j]

With this PR, these vectors are the same.

@Cryoris Cryoris added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: algorithms Related to the Algorithms module labels Jan 5, 2022
@Cryoris Cryoris requested review from manoelmarques, woodsp-ibm and a team as code owners January 5, 2022 08:45
@Cryoris Cryoris changed the title Fix parameter order in VQE, if the ansatz is resized [WIP] Fix parameter order in VQE, if the ansatz is resized Jan 5, 2022
In this test, the ansatz was resized and thus the parameters ASCII-sorted. Now they are sorted by vector, which leads to a slightly different optimization (since the params are in a different order).
@Cryoris
Copy link
Contributor Author

Cryoris commented Jan 6, 2022

Note that I had to change the TestVQE.test_max_evals_grouped test slightly since in the tests, the ansatz was resized and thus the parameters ASCII-sorted. Now they are sorted by vector, which leads to a slightly different optimization, in particular for stochastic optimizers (since the params are in a different order).

@coveralls
Copy link

coveralls commented Jan 6, 2022

Pull Request Test Coverage Report for Build 1692405590

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.0008%) to 83.101%

Files with Coverage Reduction New Missed Lines %
qiskit/algorithms/minimum_eigen_solvers/vqe.py 1 91.27%
Totals Coverage Status
Change from base Build 1687988160: -0.0008%
Covered Lines: 51984
Relevant Lines: 62555

💛 - Coveralls

@Cryoris Cryoris changed the title [WIP] Fix parameter order in VQE, if the ansatz is resized Fix parameter order in VQE, if the ansatz is resized Jan 6, 2022
@mergify mergify bot merged commit d0b26c2 into Qiskit:main Jan 13, 2022
mergify bot pushed a commit that referenced this pull request Jan 13, 2022
* fix VQE param order if ansatz is resized

* add test, rm trailing comment

* fix construct circuit for params != circuit params

* fix test_max_evals_grouped

In this test, the ansatz was resized and thus the parameters ASCII-sorted. Now they are sorted by vector, which leads to a slightly different optimization (since the params are in a different order).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d0b26c2)
mergify bot added a commit that referenced this pull request Jan 13, 2022
* fix VQE param order if ansatz is resized

* add test, rm trailing comment

* fix construct circuit for params != circuit params

* fix test_max_evals_grouped

In this test, the ansatz was resized and thus the parameters ASCII-sorted. Now they are sorted by vector, which leads to a slightly different optimization (since the params are in a different order).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d0b26c2)

Co-authored-by: Julien Gacon <[email protected]>
@mrossinek
Copy link
Member

Merging this PR caused our nightly scheduled CI of Qiskit Nature to break: https://github.com/Qiskit/qiskit-nature/actions/runs/1695769065

I verified locally that this is indeed the offending commit. I assume this has to do with the fact that this fixes a bug related to using the optimal_point? If so, then likely the expected values in our unittests were simply wrong (by a small margin as you can see from the CI output above) since the BopesSampler leverages the optimal_point when applying bootstrapping.

Tagging some more people who are not already involved in this discussion:

@Cryoris Cryoris deleted the fix-vqe-paramorder branch January 18, 2022 12:27
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* fix VQE param order if ansatz is resized

* add test, rm trailing comment

* fix construct circuit for params != circuit params

* fix test_max_evals_grouped

In this test, the ansatz was resized and thus the parameters ASCII-sorted. Now they are sorted by vector, which leads to a slightly different optimization (since the params are in a different order).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
)

* fix VQE param order if ansatz is resized

* add test, rm trailing comment

* fix construct circuit for params != circuit params

* fix test_max_evals_grouped

In this test, the ansatz was resized and thus the parameters ASCII-sorted. Now they are sorted by vector, which leads to a slightly different optimization (since the params are in a different order).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: algorithms Related to the Algorithms module stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants