Fix `NumPyEigensolver` with `SparsePauliOp` #9101

declanmillar · 2022-11-08T16:48:24Z

Summary

Fixes an issue where NumPyEigensolver and by extension NumPyMinimumEigensolver did not support all BaseOperator instances, in particular SparsePauliOp. This was because it required the data attribute, which Operator exposes but e.g. SparsePauliOp does not. Closes #9091.

Details and comments

For all BaseOperator child classes, other than Operator, to_matrix is used to get the NumPy matrix for the operator and compute the eigenvalues and eigenstates. If an Operator instance, we still use data.

Additionally this PR:

Refactors NumPyEigensolver to be stateless w.r.t. results. I.e. the result is no longer stored.
Adds unit tests to check NumPyEigensolver with SparsePauliOps.
Adds units tests to check NumPyMinimumEigensolver with SparsePauliOps and Operators.
Uses sparse matrix computation for all operators that support it, otherwise try dense computation. If this fails and error is raised.

qiskit-bot · 2022-11-08T16:48:27Z

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

coveralls · 2022-11-08T17:22:00Z

Pull Request Test Coverage Report for Build 3873960414

70 of 83 (84.34%) changed or added relevant lines in 2 files are covered.
7 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.02%) to 84.535%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/algorithms/eigensolvers/numpy_eigensolver.py	68	81	83.95%

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/hamiltonian_gate.py	1	89.23%
qiskit/extensions/unitary.py	1	95.45%
qiskit/algorithms/eigensolvers/numpy_eigensolver.py	5	84.21%

Totals
Change from base Build 3858894940:	-0.02%
Covered Lines:	64032
Relevant Lines:	75746

💛 - Coveralls

Cryoris · 2022-11-09T07:55:27Z

It might be good to try using the sparse eigensolver (and the sparse matrix multiplication in the eval_aux_ops) when possible. Right now this is implemented for the PauliSumOp, but could definitely also be done for the SparsePauliOp (or maybe even all non-Operator operators?) by using to_matrix(sparse=True) 🙂

Cryoris

LGTM, some minor comments on structure below !

qiskit/algorithms/eigensolvers/numpy_eigensolver.py

woodsp-ibm · 2022-11-14T20:54:29Z

qiskit/algorithms/eigensolvers/numpy_eigensolver.py

@@ -110,68 +108,50 @@ def _check_set_k(self, operator: BaseOperator | PauliSumOp) -> None:
                self._k = self._in_k

    def _solve(self, operator: BaseOperator | PauliSumOp) -> None:
-        if isinstance(operator, PauliSumOp):
-            sp_mat = operator.to_spmatrix()
+        if isinstance(operator, Operator):


This is a more general comment. BaseOperator | PauliSumOp was chosen to be the same as that which Estimator supports. However from what I can see BaseOperator provides no standard way to access the data comprising the operator as needed here. An Operator has data, and Pauli types seems to have to_matrix(). ScalarOp is also a BaseOperator and while it has to_matrix does not have a sparse attribute hence would fail if used here. Looking at Estimator when it does an expectation, using StateVector, it may end up creating an Operator, depending on type, where its constructor inspects the object being passed for various aspects to build it out - I think ScalarOp would pass through this. I guess I am wondering more about the if its not an Operator and then its not PauliSumOp we are in the realm of all other BaseOperators that are not the Operator subclass thereof. The to_matrix(sparse=True) looks like for the Pauli based ones like PauliTable, SparsePauliOp that this is implemented in these - but again this is a specific set of these. This logic certainly now captures the Pauli types like SparsePauliOp of BaseOperator, which it did not before. Just wondering whether we can/should do better - maybe at least wrap the to_matrix(sparse=Try) in a try/except and raise perhaps an error if that can't be called that the operator type is not supported?

It looks like a try/except is the best option, as there are several operator classes that do not subclass BasePauli but do allow sparse=True. It will also give a more appropriate error when a parameterized operator is passed in. From what I can see all BaseOperator classes, other than Operator, do expose a to_matrix method. Would it be worth adding this method to Operator and just returning data? Clifford and CNOTDihedral already effectively do this via to_operator. Could we perhaps also make to_matrix a required method of BaseOperator.

Since primitives - especially the remote ones, I would think, need access to the data to be able to do the computation the fact that BaseOperator is the designated type but provides no defined mechanisms, indeed seems problematic. I am tagging @ikkoham and @t-imamichi to bring them into the conversation more generally. The reference Estimator relies on StateVector being able to compute an estimation of the operator (it and the operators being part of quantum_info) - for other primitives, that may use an aspect of it, like here, it can be different.

Here specifically, in the near term, I think a try/except to be able to raise a more meaningful error. Longer term it seems to me that whatever the type we indicate should have suitable method(s) for the purposes we have/envision. Whether BaseOperator (or something else) is updated or perhaps new mixin/Protocol(s) added seems like it should be the subject of a separate issue/PR to discuss and hopefully address these wider needs and revisit this try/catch later as warrants.

ElePT

The code LGTM. I thought that maybe it would be possible to wrap all inputs in a SparsePauliOp not to have to deal with each case separately, but I could not find a better way to do it. I have followed a simular principle on this PR for VQD #9245.

ElePT · 2022-12-05T14:42:12Z

qiskit/algorithms/eigensolvers/numpy_eigensolver.py

@@ -109,69 +108,64 @@ def _check_set_k(self, operator: BaseOperator | PauliSumOp) -> None:
            else:
                self._k = self._in_k

-    def _solve(self, operator: BaseOperator | PauliSumOp) -> None:
+    def _solve(self, operator: BaseOperator | PauliSumOp) -> tuple[np.ndarray, np.ndarray]:


As discussed, you could add a check here for num_qubits, and raise an error if it's None, to cover all of these operator types that don't make that much sense (like ScalarOp).

ElePT · 2022-12-05T14:42:44Z

qiskit/algorithms/eigensolvers/numpy_eigensolver.py

        for key, operator in key_op_iterator:
            if operator is None:
                continue
-            value = 0.0
+
+            op_matrix = None
            if isinstance(operator, PauliSumOp):


Reminder that the check for num_qubits should also go here.

ElePT · 2022-12-05T14:45:32Z

test/python/algorithms/eigensolvers/test_numpy_eigensolver.py

-        ("ZZ", -0.01128010425623538),
-        ("XX", 0.18093119978423156),
-    ]
+from qiskit.quantum_info import Operator, SparsePauliOp


Should we add one test for other BaseOperator subclasses? I'm not sure what others would be reasonable... Pauli? I don't think it's worth it to repeat all of the tests 7 times, but having a simple test for these extra clases would probably make sense.

There are additional outliers that cause issues. For example PauliList and PauliTable, which also subclass BaseOperator but will cause an error in NumPyEigensolver. We could accommodate these, but they would require individual logic. This is easy enough, but seems like a lot of extra code for functionality that no one is likely to need.

No, I don't think it makes sense to add logic for edge cases... but we could check that the error raised is informative enough and these cases are accounted for (even if through raising an error).

ElePT

LGTM! @Cryoris, if the CI doesn't break, I would say it's mergeable 😇

* Use to_matrix instead for non-Operator BaseOperator children. * Make NumPyEigensolver stateless w.r.t. results. * Build operators as SparsePauliOps rather than PauliSumOps.

Test NumPyMinimumEigensolver with: * SparsePauliOp * Operator * PauliSumOp

* Fix NumPyEigensolver for SparsePauliOp * Use to_matrix instead for non-Operator BaseOperator children. * Make NumPyEigensolver stateless w.r.t. results. * Build operators as SparsePauliOps rather than PauliSumOps. * Test NumPyMinimumEigensolver with different ops Test NumPyMinimumEigensolver with: * SparsePauliOp * Operator * PauliSumOp * add releasenote * Use sparse matrix computation for non-Operator * Use sparse matmul for non-Operator * improve structure * try dense matrix for all BaseOperators or raise ex * remove unused import * test Operator.to_matrix() * use instance to access sparse * raise error for invalid num_qubits. pauliop/scalarop tests Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> Co-authored-by: Julien Gacon <gaconju@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

declanmillar requested review from a team, manoelmarques and woodsp-ibm as code owners November 8, 2022 16:48

declanmillar force-pushed the fix-numpy-eigensolver-with-sparse-pauli-op branch from f022fd3 to 5c133d4 Compare November 9, 2022 14:57

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

declanmillar force-pushed the fix-numpy-eigensolver-with-sparse-pauli-op branch from 5c133d4 to 7abf0c2 Compare November 9, 2022 21:12

woodsp-ibm mentioned this pull request Nov 10, 2022

Replace opflow operators with quantum_info SparsePauliOp qiskit-community/qiskit-optimization#433

Closed

Cryoris reviewed Nov 14, 2022

View reviewed changes

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

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

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

woodsp-ibm reviewed Nov 14, 2022

View reviewed changes

declanmillar force-pushed the fix-numpy-eigensolver-with-sparse-pauli-op branch from 7abf0c2 to 8f6f5b3 Compare November 15, 2022 11:21

declanmillar requested a review from ikkoham as a code owner November 17, 2022 12:30

declanmillar force-pushed the fix-numpy-eigensolver-with-sparse-pauli-op branch from 7c8374e to f106c12 Compare November 17, 2022 20:29

woodsp-ibm mentioned this pull request Nov 18, 2022

Numerical Integration methods for real and imaginary time evolution. #8399

Merged

ElePT reviewed Dec 5, 2022

View reviewed changes

declanmillar force-pushed the fix-numpy-eigensolver-with-sparse-pauli-op branch from ca03d5c to f6a37e7 Compare December 14, 2022 15:02

ElePT reviewed Dec 14, 2022

View reviewed changes

declanmillar added 10 commits December 14, 2022 16:32

Fix NumPyEigensolver for SparsePauliOp

01e4992

* Use to_matrix instead for non-Operator BaseOperator children. * Make NumPyEigensolver stateless w.r.t. results. * Build operators as SparsePauliOps rather than PauliSumOps.

Test NumPyMinimumEigensolver with different ops

4cdc22d

Test NumPyMinimumEigensolver with: * SparsePauliOp * Operator * PauliSumOp

add releasenote

347fa77

Use sparse matrix computation for non-Operator

c938100

Use sparse matmul for non-Operator

4caba64

improve structure

e9b830d

try dense matrix for all BaseOperators or raise ex

ff103be

remove unused import

a32b93b

test Operator.to_matrix()

12f2588

use instance to access sparse

9b35199

raise error for invalid num_qubits. pauliop/scalarop tests

1beb7c4

declanmillar force-pushed the fix-numpy-eigensolver-with-sparse-pauli-op branch from f6a37e7 to 1beb7c4 Compare December 14, 2022 16:32

ElePT and others added 2 commits December 19, 2022 12:14

Merge branch 'main' into fix-numpy-eigensolver-with-sparse-pauli-op

aafd5d0

Merge branch 'main' into fix-numpy-eigensolver-with-sparse-pauli-op

629f9ba

Cryoris added this to the 0.23.0 milestone Dec 20, 2022

Cryoris approved these changes Jan 9, 2023

View reviewed changes

Cryoris added the automerge label Jan 9, 2023

Merge branch 'main' into fix-numpy-eigensolver-with-sparse-pauli-op

31fb61f

mergify bot merged commit 9b58d41 into Qiskit:main Jan 9, 2023

declanmillar deleted the fix-numpy-eigensolver-with-sparse-pauli-op branch January 17, 2023 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `NumPyEigensolver` with `SparsePauliOp` #9101

Fix `NumPyEigensolver` with `SparsePauliOp` #9101

declanmillar commented Nov 8, 2022 •

edited

Loading

qiskit-bot commented Nov 8, 2022

coveralls commented Nov 8, 2022 •

edited

Loading

Cryoris commented Nov 9, 2022

Cryoris left a comment

woodsp-ibm Nov 14, 2022

declanmillar Nov 15, 2022

woodsp-ibm Nov 15, 2022

ElePT left a comment •

edited

Loading

ElePT Dec 5, 2022

ElePT Dec 5, 2022

ElePT Dec 5, 2022

declanmillar Dec 6, 2022

ElePT Dec 7, 2022

ElePT left a comment •

edited

Loading

Fix NumPyEigensolver with SparsePauliOp #9101

Fix NumPyEigensolver with SparsePauliOp #9101

Conversation

declanmillar commented Nov 8, 2022 • edited Loading

Summary

Details and comments

qiskit-bot commented Nov 8, 2022

coveralls commented Nov 8, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3873960414

💛 - Coveralls

Cryoris commented Nov 9, 2022

Cryoris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElePT left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElePT left a comment • edited Loading

Choose a reason for hiding this comment

Fix `NumPyEigensolver` with `SparsePauliOp` #9101

Fix `NumPyEigensolver` with `SparsePauliOp` #9101

declanmillar commented Nov 8, 2022 •

edited

Loading

coveralls commented Nov 8, 2022 •

edited

Loading

ElePT left a comment •

edited

Loading

ElePT left a comment •

edited

Loading