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 VQD's optimal_values #10279

Merged
merged 7 commits into from
Jun 20, 2023
Merged

Fix VQD's optimal_values #10279

merged 7 commits into from
Jun 20, 2023

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jun 14, 2023

Summary

Fixes #10263

Details and comments

I think that there should be a unit test checking this, will add soon.

@ElePT ElePT requested review from a team and woodsp-ibm as code owners June 14, 2023 13:09
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @ElePT
  • @Qiskit/terra-core
  • @woodsp-ibm

@woodsp-ibm
Copy link
Member

I think that there should be a unit test checking this, will add soon.

And a bug fix release note

@coveralls
Copy link

coveralls commented Jun 14, 2023

Pull Request Test Coverage Report for Build 5289745616

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 48 unchanged lines in 11 files lost coverage.
  • Overall coverage increased (+0.03%) to 85.966%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
qiskit/circuit/quantumcircuit.py 1 94.08%
qiskit/circuit/tools/pi_check.py 1 91.23%
crates/qasm2/src/lex.rs 3 90.38%
qiskit/pulse/library/waveform.py 3 93.75%
crates/accelerate/src/sabre_swap/layer.rs 4 97.32%
qiskit/primitives/estimator.py 4 94.87%
crates/qasm2/src/parse.rs 6 97.58%
qiskit/primitives/base/base_estimator.py 6 90.0%
qiskit/primitives/utils.py 8 88.61%
Totals Coverage Status
Change from base Build 5266466291: 0.03%
Covered Lines: 71488
Relevant Lines: 83158

💛 - Coveralls

@@ -200,6 +200,24 @@ def store_intermediate_result(eval_count, parameters, mean, metadata, step):
np.testing.assert_array_almost_equal(history["mean"], ref_mean, decimal=2)
np.testing.assert_array_almost_equal(history["step"], ref_step, decimal=0)

@data(H2_PAULI, H2_OP, H2_SPARSE_PAULI)
def test_optimal_values(self, op):
"""Test running same VQD twice to re-use optimizer, then switch optimizer"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description here seems to be a copy/paste from the following test, but...

...I guess the check for this field could have been added as a subtest under the test_basic_operator above that seems to have subtests for a bunch of the result fields, rather than a new test which has to run the VQD computation again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you're totally right, I overlooked that. Fixed in bfa1bac

@woodsp-ibm woodsp-ibm 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 Jun 16, 2023
@woodsp-ibm woodsp-ibm added this to the 0.24.2 milestone Jun 16, 2023
@ElePT ElePT added this pull request to the merge queue Jun 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2023
@ElePT ElePT added this pull request to the merge queue Jun 20, 2023
Merged via the queue into Qiskit:main with commit 93a8172 Jun 20, 2023
13 checks passed
mergify bot pushed a commit that referenced this pull request Jun 20, 2023
* Fix optimal values

* Add unittest

* Add reno

* Update unittest

* Revert "Update unittest"

This reverts commit 5000d2f.

* Update test

* Make test subtest

(cherry picked from commit 93a8172)
jakelishman pushed a commit that referenced this pull request Jun 20, 2023
* Fix optimal values

* Add unittest

* Add reno

* Update unittest

* Revert "Update unittest"

This reverts commit 5000d2f.

* Update test

* Make test subtest

(cherry picked from commit 93a8172)

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
ElePT added a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* Fix optimal values

* Add unittest

* Add reno

* Update unittest

* Revert "Update unittest"

This reverts commit 225cd09e1ae08cd48f5196fd1d3a87a549f0164a.

* Update test

* Make test subtest
ElePT added a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
* Fix optimal values

* Add unittest

* Add reno

* Update unittest

* Revert "Update unittest"

This reverts commit 5000d2fb02e8126b376d9cd30d2862c202af2755.

* Update test

* Make test subtest
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.

VQD optimal_values result field is incorrect
4 participants