Fix `betas` calculation on `VQD` and async. cost evaluation #9245

ElePT · 2022-12-05T15:50:36Z

Summary

This PR fixes the betas auto-evaluation on VQD for PauliSumOp operators (pending deprecation), and adds support for beta auto-evaluation with SparsePauliOp. The code was supposed to iterate over primitive coefficients, and was instead obtaining a series of PauliSumOp coefficients (which were always 1.0).

In addition to the betas fix, this PR includes a change in the energy eval. method to leverage the async capabilities of the primitives (the job results are retrieved after both jobs are submitted, instead of sequentially submitting and retrieving results), as this was the initial intention.

Details and comments

The unit tests have also been updated not to use PauliSumOp internally, and to test SparsePauliOp imports.

qiskit-bot · 2022-12-05T15:50:44Z

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:

@Qiskit/terra-core
@manoelmarques
@woodsp-ibm

woodsp-ibm · 2022-12-05T16:30:32Z

This should have a reno and we probably want to release this in a bug fix release too right?

coveralls · 2022-12-05T17:12:12Z

Pull Request Test Coverage Report for Build 3632367696

17 of 20 (85.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.001%) to 84.599%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/algorithms/eigensolvers/vqd.py	17	20	85.0%

Files with Coverage Reduction	New Missed Lines	%
qiskit/algorithms/eigensolvers/vqd.py	1	96.23%

Totals
Change from base Build 3632367272:	0.001%
Covered Lines:	63154
Relevant Lines:	74651

💛 - Coveralls

Cryoris · 2022-12-05T20:54:13Z

qiskit/algorithms/eigensolvers/vqd.py

@@ -227,15 +228,19 @@ def compute_eigenvalues(
        if self.betas is None:
            if isinstance(operator, PauliSumOp):
                upper_bound = abs(operator.coeff) * sum(
-                    abs(operation.coeff) for operation in operator
+                    abs(operation.primitive.coeffs) for operation in operator


Could this simplified to the following?

Suggested change

abs(operation.primitive.coeffs) for operation in operator

np.abs(operator.primitive.coeffs)

Or maybe even just cast to a SparsePauliOp for the whole function?

if isinstance(operator, PauliSumOp): operator = operator.coeff * operator.primitive

qiskit/algorithms/eigensolvers/vqd.py

Cryoris · 2022-12-05T20:57:36Z

qiskit/algorithms/eigensolvers/vqd.py

+
+            try:
+                estimator_result = estimator_job.result()
+                values = estimator_result.values + total_cost


Not blocking but would it be better to take the summation out of the try block to distinguish between the estimator failing and the addition failing (due to e.g. wrong types, or the result being None,...)?

Cryoris · 2022-12-05T21:00:22Z

test/python/algorithms/eigensolvers/test_vqd.py

@@ -116,7 +115,16 @@ def test_full_spectrum(self):
            result.eigenvalues.real, self.h2_energy_excited, decimal=2
        )

-    @data(H2_PAULI, H2_OP)
+    def test_betas_autoeval(self):


Is there a way we could test the autoevaluation itself? This here would also fail if something else in the algorithm is going wrong 🙂 What about e.g. checking the log?

Yeah... fair enough, I did check the log but didn't include it in the test.

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

qiskit/algorithms/eigensolvers/vqd.py

test/python/algorithms/eigensolvers/test_vqd.py

Cryoris

LGTM thanks for the fix!

* Fix betas * Add async * Fix seed test * Fix lint * Add reno * Add enumerate Co-authored-by: Julien Gacon <[email protected]> * Apply suggestions Julien * Fix lint Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit f339f71)

…9253) * Fix betas * Add async * Fix seed test * Fix lint * Add reno * Add enumerate Co-authored-by: Julien Gacon <[email protected]> * Apply suggestions Julien * Fix lint Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit f339f71) Co-authored-by: ElePT <[email protected]> Co-authored-by: Julien Gacon <[email protected]>

) * Fix betas * Add async * Fix seed test * Fix lint * Add reno * Add enumerate Co-authored-by: Julien Gacon <[email protected]> * Apply suggestions Julien * Fix lint Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

…iskit#9245) * Fix betas * Add async * Fix seed test * Fix lint * Add reno * Add enumerate Co-authored-by: Julien Gacon <[email protected]> * Apply suggestions Julien * Fix lint Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

ElePT added 2 commits December 5, 2022 16:15

Fix betas

8f8031f

Add async

d63e6ef

ElePT requested review from a team, manoelmarques and woodsp-ibm as code owners December 5, 2022 15:50

ElePT changed the title ~~Fix betas calculation for~~ Fix betas calculation on VQD and add async. Dec 5, 2022

Fix seed test

7eecca1

woodsp-ibm added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: algorithms Related to the Algorithms module labels Dec 5, 2022

ElePT changed the title ~~Fix betas calculation on VQD and add async.~~ Fix betas calculation on VQD and async. cost evaluation Dec 5, 2022

ElePT added 2 commits December 5, 2022 17:35

Fix lint

71630cc

Add reno

9f68e26

Cryoris reviewed Dec 5, 2022

View reviewed changes

qiskit/algorithms/eigensolvers/vqd.py Outdated Show resolved Hide resolved

Cryoris reviewed Dec 5, 2022

View reviewed changes

ElePT and others added 2 commits December 6, 2022 11:14

Add enumerate

44d8816

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

Apply suggestions Julien

c6acc67

Cryoris reviewed Dec 6, 2022

View reviewed changes

qiskit/algorithms/eigensolvers/vqd.py Outdated Show resolved Hide resolved

Cryoris reviewed Dec 6, 2022

View reviewed changes

test/python/algorithms/eigensolvers/test_vqd.py Outdated Show resolved Hide resolved

Fix lint

6998fe5

Cryoris approved these changes Dec 6, 2022

View reviewed changes

Cryoris added stable backport potential The bug might be minimal and/or import enough to be port to stable automerge labels Dec 6, 2022

Merge branch 'main' into fix-vqd-betas

0194919

mergify bot merged commit f339f71 into Qiskit:main Dec 6, 2022

mergify bot mentioned this pull request Dec 6, 2022

Fix betas calculation on VQD and async. cost evaluation (backport #9245) #9253

Merged

ElePT deleted the fix-vqd-betas branch December 7, 2022 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `betas` calculation on `VQD` and async. cost evaluation #9245

Fix `betas` calculation on `VQD` and async. cost evaluation #9245

ElePT commented Dec 5, 2022 •

edited

Loading

qiskit-bot commented Dec 5, 2022

woodsp-ibm commented Dec 5, 2022

coveralls commented Dec 5, 2022 •

edited

Loading

Cryoris Dec 5, 2022

Cryoris Dec 5, 2022

ElePT Dec 6, 2022

Cryoris Dec 5, 2022

ElePT Dec 6, 2022 •

edited

Loading

Cryoris left a comment

	abs(operation.primitive.coeffs) for operation in operator
	np.abs(operator.primitive.coeffs)

Fix betas calculation on VQD and async. cost evaluation #9245

Fix betas calculation on VQD and async. cost evaluation #9245

Conversation

ElePT commented Dec 5, 2022 • edited Loading

Summary

Details and comments

qiskit-bot commented Dec 5, 2022

woodsp-ibm commented Dec 5, 2022

coveralls commented Dec 5, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3632367696

💛 - Coveralls

Cryoris Dec 5, 2022

Choose a reason for hiding this comment

Cryoris Dec 5, 2022

Choose a reason for hiding this comment

ElePT Dec 6, 2022

Choose a reason for hiding this comment

Cryoris Dec 5, 2022

Choose a reason for hiding this comment

ElePT Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Cryoris left a comment

Choose a reason for hiding this comment

Fix `betas` calculation on `VQD` and async. cost evaluation #9245

Fix `betas` calculation on `VQD` and async. cost evaluation #9245

ElePT commented Dec 5, 2022 •

edited

Loading

coveralls commented Dec 5, 2022 •

edited

Loading

ElePT Dec 6, 2022 •

edited

Loading