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.

Sign up for GitHub

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

Jump to bottom

Fix VQD with SPSA optimizer #9538

Merged
merged 19 commits into from
Feb 17, 2023
Merged

Fix VQD with SPSA optimizer #9538

merged 19 commits into from
Feb 17, 2023

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Feb 6, 2023 •
edited
Loading

Summary

Fixes #9500, which failed because SPSA would call the energy evaluation with batches of parameter vectors, but VQD did not know hot to handle parameter batches (raised TypeError).

I will also update the unit tests to include tests with SPSA.

Details and comments

@ElePT ElePT added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: algorithms Related to the Algorithms module labels Feb 6, 2023
@ElePT ElePT self-assigned this Feb 6, 2023
@ElePT ElePT requested review from a team, manoelmarques and woodsp-ibm as code owners February 6, 2023 14:47
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

Cryoris
Cryoris reviewed Feb 6, 2023
View reviewed changes
qiskit/algorithms/eigensolvers/vqd.py Outdated Show resolved Hide resolved
test/python/algorithms/eigensolvers/test_vqd.py Outdated Show resolved Hide resolved
test/python/algorithms/eigensolvers/test_vqd.py Outdated Show resolved Hide resolved
@woodsp-ibm
Copy link
Member

In a comment in the issue here #9500 (comment) I mentioned that explicitly setting the grouping externally on the optimizer (e.g as abelow) fails. The error was different than I got with SPSA. Can we add a unit test that covers this explicit grouping setting - not quite sure why it was a different error raised than with SPSA.

optimizer = SLSQP(maxiter=60, max_evals_grouped=10)

ElePT
ElePT commented Feb 15, 2023
View reviewed changes
qiskit/algorithms/eigensolvers/vqd.py Outdated Show resolved Hide resolved
qiskit/algorithms/eigensolvers/vqd.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 16, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 4206157941

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 85.278%
Totals Coverage Status
Change from base Build 4204708183: 0.002%
Covered Lines: 67250
Relevant Lines: 78860
💛 - Coveralls

Cryoris
Cryoris reviewed Feb 16, 2023
View reviewed changes
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

One tiny comment on the reno link otherwise LGTM!

@Cryoris Cryoris added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Feb 16, 2023
@ElePT @Cryoris
Apply reno suggestion
ac03e12 
Co-authored-by: Julien Gacon <[email protected]>
@ElePT ElePT requested a review from Cryoris February 17, 2023 13:00
Cryoris
Cryoris approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! 🙂

@mergify mergify bot merged commit 061aee2 into Qiskit:main Feb 17, 2023
mergify bot pushed a commit that referenced this pull request Feb 17, 2023
@ElePT @mergify
Fix VQD with SPSA optimizer (#9538)
d6194a4 
* Add parameter batching

* Add spsa to tests

* Add reno

* Remove batch size from evals

* Apply suggestions from code review

* Apply suggestion from code review

* Update tests

* Delete Untitled.ipynb

* Fix batch size SPSA

* Small fixes

* fix batching

* update test

* Update reno

* Change SPSA test

* Apply reno suggestion

Co-authored-by: Julien Gacon <[email protected]>

---------

Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 061aee2)
@mergify mergify bot mentioned this pull request Feb 17, 2023
Merged
mergify bot added a commit that referenced this pull request Feb 17, 2023
@mergify @ElePT
Fix VQD with SPSA optimizer (#9538) (#9613)
b5c5761 
* Add parameter batching

* Add spsa to tests

* Add reno

* Remove batch size from evals

* Apply suggestions from code review

* Apply suggestion from code review

* Update tests

* Delete Untitled.ipynb

* Fix batch size SPSA

* Small fixes

* fix batching

* update test

* Update reno

* Change SPSA test

* Apply reno suggestion

Co-authored-by: Julien Gacon <[email protected]>

---------

Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 061aee2)

Co-authored-by: ElePT <[email protected]>
ElePT added a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
@ElePT @Cryoris @mergify
Fix VQD with SPSA optimizer (Qiskit#9538)
4b25e35 
* Add parameter batching

* Add spsa to tests

* Add reno

* Remove batch size from evals

* Apply suggestions from code review

* Apply suggestion from code review

* Update tests

* Delete Untitled.ipynb

* Fix batch size SPSA

* Small fixes

* fix batching

* update test

* Update reno

* Change SPSA test

* Apply reno suggestion

Co-authored-by: Julien Gacon <[email protected]>

---------

Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT added a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
@ElePT @Cryoris @mergify
Fix VQD with SPSA optimizer (Qiskit/qiskit#9538)
f5ca4b4 
* Add parameter batching

* Add spsa to tests

* Add reno

* Remove batch size from evals

* Apply suggestions from code review

* Apply suggestion from code review

* Update tests

* Delete Untitled.ipynb

* Fix batch size SPSA

* Small fixes

* fix batching

* update test

* Update reno

* Change SPSA test

* Apply reno suggestion

Co-authored-by: Julien Gacon <[email protected]>

---------

Co-authored-by: Julien Gacon <[email protected]>
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
Reviewers

@Cryoris Cryoris Cryoris approved these changes

@manoelmarques manoelmarques Awaiting requested review from manoelmarques

@woodsp-ibm woodsp-ibm Awaiting requested review from woodsp-ibm

Assignees

@ElePT ElePT

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

VQD fails when using SPSA optimizer
5 participants
@ElePT @qiskit-bot @woodsp-ibm @coveralls @Cryoris