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

Use np.array_split instead of np.split to support uneven splits in spsa._batch_evaluate #8634

Merged
merged 7 commits into from
Aug 31, 2022

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Aug 30, 2022

Fixes qiskit-community/qiskit-nature#797

Summary

The internal function qiskit.algorithms.optimizers.spsa._batch_evaluate uses np.split instead of np.array_split. This results in an exception as described here when the split is uneven.

Details and comments

I used the static method qiskit.algorithms.SPSA.estimate_stddev for testing, since it was not directly tested and the method made it easier to parametrize in a way to cover the fix. However, I'm not sure if my input for that test is making any sense.

@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:

@1ucian0 1ucian0 added Changelog: Bugfix Include in the "Fixed" section of the changelog priority: high labels Aug 30, 2022
@coveralls
Copy link

coveralls commented Aug 30, 2022

Pull Request Test Coverage Report for Build 2966979548

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.147%

Totals Coverage Status
Change from base Build 2965786691: 0.0%
Covered Lines: 57022
Relevant Lines: 67765

💛 - Coveralls

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Aug 30, 2022

@Cryoris can you take at look at this please. Since this is splitting multiple points to be evaluated there should never be an uneven split afaik since its a list of points each point being the same length.

@Cryoris
Copy link
Contributor

Cryoris commented Aug 30, 2022

There can be uneven splits especially since SPSA does the calibration of the learning rate in the beginning, which uses 50 function evaluations. So if a user specifies e.g. max_evals_grouped=3, then we'd end up with 16 groups of 3 points and one group with 2 points. I already gave Luciano feedback offline 👍🏻

Co-authored-by: Julien Gacon <gaconju@gmail.com>
1ucian0 and others added 2 commits August 30, 2022 19:01
@1ucian0 1ucian0 added this to the 0.22 milestone Aug 31, 2022
Cryoris
Cryoris previously approved these changes Aug 31, 2022
@Cryoris Cryoris added stable backport potential The bug might be minimal and/or import enough to be port to stable automerge labels Aug 31, 2022
@mergify mergify bot merged commit 69ad6c6 into Qiskit:main Aug 31, 2022
mergify bot pushed a commit that referenced this pull request Aug 31, 2022
…sa._batch_evaluate (#8634)

* Use np.array_split instead of np.split to support uneven splits in spsa._batch_evaluate

* better testing

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update test/python/algorithms/optimizers/test_spsa.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* len of a boolean

* Apply suggestions from code review

* Fix Sphinx ref

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 69ad6c6)
mergify bot added a commit that referenced this pull request Aug 31, 2022
…sa._batch_evaluate (#8634) (#8655)

* Use np.array_split instead of np.split to support uneven splits in spsa._batch_evaluate

* better testing

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update test/python/algorithms/optimizers/test_spsa.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* len of a boolean

* Apply suggestions from code review

* Fix Sphinx ref

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 69ad6c6)

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
…sa._batch_evaluate (Qiskit#8634)

* Use np.array_split instead of np.split to support uneven splits in spsa._batch_evaluate

* better testing

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update test/python/algorithms/optimizers/test_spsa.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* len of a boolean

* Apply suggestions from code review

* Fix Sphinx ref

Co-authored-by: Julien Gacon <gaconju@gmail.com>
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
…sa._batch_evaluate (Qiskit/qiskit#8634)

* Use np.array_split instead of np.split to support uneven splits in spsa._batch_evaluate

* better testing

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update test/python/algorithms/optimizers/test_spsa.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* len of a boolean

* Apply suggestions from code review

* Fix Sphinx ref

Co-authored-by: Julien Gacon <gaconju@gmail.com>
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 priority: high 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.

VQEClient failed unexpectedly
5 participants