Support Dict[str, OperatorBase] for aux_operators (fix #6772) #6870

CisterMoke · 2021-08-04T23:59:23Z

Summary

A fix for #6772
The aux_operators argument defined in the MinimumEigenSolver and EigenSolver interfaces are now of type Dict[str, Optional[OperatorBase]] instead of List[Optional[OperatorBase]]

Changelog

The EigenSolver and MinimumEigenSolver subclasses now support dictionaries for the aux_operators parameter in compute_eigenvalues and compute_minimum_eigenvalues.
Unittests for NumPyMinimumEigenSolver and VQE were added to test the dictionary support.
The VQE no longer sets the aux_operator_eigenvalues of the MinimumEigensolverResult to an ndarray.

CLAassistant · 2021-08-04T23:59:29Z

All committers have signed the CLA.

woodsp-ibm · 2021-08-05T20:48:16Z

We need to maintain the ability to pass a List in, code does this now and will break if its not supported. My preference would be just to add the dictionary support so a user can choose to pass either; but if people feel strongly the List support could be deprecated for later removal. So List in means List of values back as it is today, Dict in means Dict of values back as a result.

For the Dictionary I would type it Dict[Any, OperatorBase]. Allow users to use whatever keys they prefer - which need not be strings. Also one rationale of going to a dictionary was not to have to deal with None. When we create aux_ops we have a list and the creator knows how to handle a list of results, where the results are in the same order. As operators get processed down the stack we can do things like symmetry reduction where if an aux_op does not commute we need to 'discard' it from being measured. When we had ordered Lists the only thing we could do was put None in its place and None back in result, so that the originator, when they look through the list of values knows how to match them. They have to be careful of None evidently. The idea with the dictionary was that we would just drop the whole key/value pair say if symmetry demanded, and hence in the return the originator will just see missing key(s). They need to account for missing keys but its arguably simpler/neater this was than preserving order in a List with None(s).

CisterMoke · 2021-08-05T21:21:57Z

Thank you for the feedback! I'll restore the support for lists and drop Nones from the dicts

mrossinek

Besides these comments I think we need some unittests which ensure that this is working as expected. Just as a basic example you could look at this test and then add a case where self.aux_ops would be a dict instead of a list.

mrossinek · 2021-08-09T06:30:12Z

qiskit/algorithms/eigen_solvers/eigen_solver.py


 import numpy as np
 from qiskit.opflow import OperatorBase
 from ..algorithm_result import AlgorithmResult

+# Introduced new type to maintain readability.
+T = TypeVar('T')
+ListOrDict = Union[List[Optional[T]], Dict[Any, T]]


I think the following should be fine in this scenario:

Suggested change

ListOrDict = Union[List[Optional[T]], Dict[Any, T]]

ListOrDict = Union[List[Optional[T]], Dict[str, T]]

The author originally had str as a typehint for key for dict. I saw no real reason why we needed to limit the user to requiring to provide strings as keys. After all I may find different keys more convenient for my application/usage so in my mind there was no need to limit this.

mrossinek · 2021-08-09T06:33:44Z

qiskit/algorithms/eigen_solvers/numpy_eigen_solver.py

            zero_op = I.tensorpower(operator.num_qubits) * 0.0
            # For some reason Chemistry passes aux_ops with 0 qubits and paulis sometimes.
            aux_operators = [zero_op if op == 0 else op for op in aux_operators]
+        elif isinstance(aux_operators, dict) and len(aux_operators) > 0:
+            aux_operators = {key: op for key, op in aux_operators.items() if not op}


Why do check if not op at the end? Shouldn't this be if op? Or am I misunderstanding something here?

Well spotted, the not should not be there

mrossinek · 2021-08-09T06:34:09Z

qiskit/algorithms/minimum_eigen_solvers/minimum_eigen_solver.py


 import numpy as np
 from qiskit.opflow import OperatorBase
 from ..algorithm_result import AlgorithmResult

+# Introduced new type to maintain readability.
+T = TypeVar('T')
+ListOrDict = Union[List[Optional[T]], Dict[Any, T]]


Just as before:

Suggested change

ListOrDict = Union[List[Optional[T]], Dict[Any, T]]

ListOrDict = Union[List[Optional[T]], Dict[str, T]]

Similar comment to above.

CisterMoke · 2021-08-09T20:33:54Z

Besides these comments I think we need some unittests which ensure that this is working as expected. Just as a basic example you could look at this test and then add a case where self.aux_ops would be a dict instead of a list.

Alright, I'll start writing some unittests! On top of that I'll also make sure that the Lint tests pass.

CisterMoke · 2021-08-10T19:11:21Z

test/python/algorithms/test_numpy_minimum_eigen_solver.py

+        self.assertEqual(len(result.aux_operator_eigenvalues), 2)
+        np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues["aux_op1"], [2, 0])
+        np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues["aux_op2"], [0, 0])
+



@mrossinek I've only added a test for the NumPyMinimumEigenSolver since I did not feel comfortable with adding one for the VQE due to the probabilistic nature. Does this suffice?

As a comment we have the algorithm_globals random seed, plus there are simulator and transpiler seeds. You can see these set on a number of tests where random functions are involved to allow the tests to be reproducible. And for VQE the statevector simulation can be used too to get the ideal outcome.

I think we also need some tests for VQE because it does not reuse the entire code from the NumPyMinimumEigensolver. Steve mentioned above how you can avoid issues with the random nature of the tests.

mrossinek

Thanks @CisterMoke! I still have some comments but other than that this is going in a great direction! 🙂

mrossinek · 2021-08-11T12:29:08Z

qiskit/algorithms/minimum_eigen_solvers/vqe.py

+        # # Deal with the aux_op behavior where there can be Nones or Zero qubit Paulis in the list
+        # aux_operator_eigenvalues = {
+        #     key: None if aux_operators[key] is None else result
+        #     for key, result in aux_op_results.items()
+        # }


Why do you leave a copy of the old code here?

I kept it as a reference while editing but forgot to remove it after I was finished. Will be removed in the next commit

mrossinek · 2021-08-11T12:30:57Z

qiskit/algorithms/minimum_eigen_solvers/vqe.py

-            result.aux_operator_eigenvalues = aux_values[0]
+            result.aux_operator_eigenvalues = aux_values


I am not exactly sure why this was only storing the first entry.. @woodsp-ibm do you know this?

In looking it seems to be the weird way it was built out. Starts out with a List which it then nests, when it does an np.array as below, and then in the above it unnested it
https://github.com/Qiskit/qiskit-terra/blob/bd537b161ce6c06e929e623f91d46fe54a61f00b/qiskit/algorithms/minimum_eigen_solvers/vqe.py#L429-L436

mrossinek · 2021-08-11T12:32:50Z

test/python/algorithms/test_numpy_minimum_eigen_solver.py

+        self.assertEqual(len(result.aux_operator_eigenvalues), 2)
+        np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues["aux_op1"], [2, 0])
+        np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues["aux_op2"], [0, 0])
+



I think we also need some tests for VQE because it does not reuse the entire code from the NumPyMinimumEigensolver. Steve mentioned above how you can avoid issues with the random nature of the tests.

CisterMoke · 2021-08-11T21:32:47Z

While I was writing a test for the VQE I noticed that the aux_operator_eigenvalues of the MinimumEigensolverResult is a plain list or dict for the NumPyMinimumEigensolver since it takes the first element of an ndarray in compute_minimum_eigenvalue.
For the VQE, however, aux_operator_eigenvalues is set to an ndarray with it's only element being a list or dict of the eigenvalues (line 452).
Am I correct to assume that the ndarray in VQE can be omitted?

mrossinek

Thanks @CisterMoke! I still have some minor comments but other than that this is looking great!

mrossinek · 2021-08-20T08:10:56Z

qiskit/algorithms/minimum_eigen_solvers/minimum_eigen_solver.py

@@ -94,11 +98,11 @@ def eigenstate(self, value: np.ndarray) -> None:
        self._eigenstate = value

    @property
-    def aux_operator_eigenvalues(self) -> Optional[np.ndarray]:
+    def aux_operator_eigenvalues(self) -> Optional[ListOrDict[Union[float, complex]]]:


Suggested change

def aux_operator_eigenvalues(self) -> Optional[ListOrDict[Union[float, complex]]]:

def aux_operator_eigenvalues(self) -> Optional[ListOrDict[complex]]:

I think complex implies float

The same change can be done elsewhere

Here I was a bit puzzled on the actual type of the eigenvalues. For the main operator it seems to be complex number but for the auxiliary operators it seem to be only real numbers.
After taking another look at _eval_aux_operators of NumPyEigenSolver and _eval_aux_ops of VQE it seems that the the first returns a Tuple[float, float] and the second return just a float.
Any suggestions which one to pick? Or should these both be cast to complex numbers?

There is no need to cast them. You can pass a float to a function which expects complex because these are subtypes

mrossinek · 2021-08-20T08:13:52Z

qiskit/algorithms/minimum_eigen_solvers/vqe.py

        # Create new CircuitSampler to avoid breaking existing one's caches.
        sampler = CircuitSampler(self.quantum_instance)

-        aux_op_meas = expectation.convert(StateFn(ListOp(aux_operators), is_measurement=True))
+        if isinstance(aux_operators, dict):
+            list_op = ListOp(list(aux_operators.values()))


Do we need to use an OrderedDict to ensure that this does not break?

Starting from 3.6 a standard Python dict maintains the insertion order so I thought it would not be necessary to use an OrderedDict
https://stackoverflow.com/a/39537308/13115502

Thanks, that's very good to know! 👍

mrossinek

I think this looks good now! Thanks @CisterMoke for working on this!

@woodsp-ibm what do you think?

qiskit/algorithms/minimum_eigen_solvers/vqe.py

mrossinek

Thanks for the additional updates @CisterMoke! 👍

woodsp-ibm

Thanks for making this contribution - its something we have been wanting to have for a while so its great to finally have it!

Realised it needs a reno release note

woodsp-ibm · 2021-09-08T21:16:00Z

@CisterMoke One last thing - I forgot until just after approving so I dismissed my approval - can you please an a reno release note that describes this new feature

mrossinek

Thanks for adding the reno, too! 👍

woodsp-ibm · 2021-09-14T19:27:31Z

Thanks for the reno - in the meanwhile it seems that another change is causing a conflict with vqe. If that can be sorted then this is good to go I think.

CisterMoke · 2021-09-14T21:45:56Z

qiskit/algorithms/minimum_eigen_solvers/vqe.py

@@ -517,7 +532,7 @@ def compute_minimum_eigenvalue(

        if aux_operators is not None:
            aux_values = self._eval_aux_ops(opt_params, aux_operators, expectation=expectation)


Thanks for the reno - in the meanwhile it seems that another change is causing a conflict with vqe. If that can be sorted then this is good to go I think.

@woodsp-ibm The conflict is the following

<<<<<<< issue6772/dict_support_for_aux_ops aux_values = self._eval_aux_ops(opt_params, aux_operators, expectation=expectation) result.aux_operator_eigenvalues = aux_values ======= aux_values = self._eval_aux_ops(opt_result.x, aux_operators, expectation=expectation) result.aux_operator_eigenvalues = aux_values[0] >>>>>>> main

where opt_result is obtained from the new self.optimizer.minimize.

I've locally changed the old opt_params to the new opt_result.x but this leads to nearly all VQE tests failing with NoneType errors. The culprit is that when self.optimizer.minimize encounters an error, it falls back to the deprecated self.optimizer.optimize. The results are stored in the opt_result variable but this is a tuple instead of an object with attributes (lines 505-511) so that opt_result.x returns None.

Any suggestions on what should be done next?

Could you specify in a bit more detail what breaks? I just tried merging main into your branch locally and using the following:

aux_values = self._eval_aux_ops(opt_result.x, aux_operators, expectation=expectation) result.aux_operator_eigenvalues = aux_values

The unittests in test/python/algorithms/test_vqe.py pass just fine. I am not sure whether I am misunderstanding the error you were facing or whether this problem has been resolved in the past 6 days.

That is good to hear, thank you for testing. It is probably an issue with my local repository then since the tests also fail when I'm simply on the main branch after updating it to the upstream main branch.

Just to be safe I'll merge via github instead of my local git repo.
Below is an excerpt of the errors I get from pytest.

test\python\algorithms\test_vqe.py FFFFFFFFFFFFF.FFFssssssssF [100%] _____ TestVQE.test_aux_operators_dict ______ 'NoneType' object is not iterable During handling of the above exception, another exception occurred: NOTE: Incompatible Exception Representation, displaying natively: testtools.testresult.real._StringException: Traceback (most recent call last): File "C:\QuantumComputing\qiskit-terra\qiskit\algorithms\minimum_eigen_solvers\vqe.py", line 508, in compute_minimum_eigenvalue opt_result = self.optimizer.minimize( File "C:\QuantumComputing\qiskit-terra\qiskit\algorithms\optimizers\scipy_optimizer.py", line 129, in minimize raw_result = minimize( (skipping part of the stack trace) File "C:\QuantumComputing\qiskit-terra\qiskit\dagcircuit\dagcircuit.py", line 1073, in substitute_node_with_dag node_map = self._multi_graph.substitute_node_with_subgraph( AttributeError: 'PyDAG' object has no attribute 'substitute_node_with_subgraph'

mrossinek

Thanks for fixing the merge conflict!

woodsp-ibm

Thanks again for doing this and sorting out the conflict. LGTM.

mrossinek · 2021-09-29T13:09:28Z

Hi @CisterMoke! This PR is approved but cannot be merged because it is not up-to-date with the base branch. Please ensure that the following checkbox in the right column is ticked:

This will allow us to keep the brach up-to-date and get this merged.

Cryoris

I only have one comment below, otherwise LGTM.

It's good that this changes the return type of the aux_values to a list instead of an object-array -- this was already changed in the runtime since the object-arrays raised problems with the serialization.

Cryoris · 2021-10-06T07:43:18Z

qiskit/algorithms/eigen_solvers/eigen_solver.py


 import numpy as np
 from qiskit.opflow import OperatorBase
 from ..algorithm_result import AlgorithmResult

+# Introduced new type to maintain readability.
+_T = TypeVar("_T")  # Pylint does not allow single character class names.
+ListOrDict = Union[List[Optional[_T]], Dict[Any, _T]]


Would it be valid to restrict the dictionary key to a str, or does it have to be Any? If users want to use the VQE runtime and pass the aux ops as dictionary, it'll have to be serializable which is not guaranteed if the key can be anything.

Speaking for the purposes of the Qiskit Nature applications I believe str should be sufficient 👍

It was discussed earlier #6870 (comment) but the topic of serialization did not arise. It seemed more flexible not to have to constrain key type; of course for serialization in the runtime one could come up with a set of uniques strings and map them on input/output to whatever keys were defined locally by the user - more work of course. I guess it would be easier to go from str to Any down the road if people object to the limitation so if for now str facilitates the runtime I guess we can go with that.

Thanks! I had forgotten about our previous discussion...

@CisterMoke could you please address this suggestion and change Any to str for the typehint in ListOrDict everywhere?
After that I believe we should be able to proceed with merging this 👍

Hi, sorry for the late update! I had kind of forgotten about this pull request at this point 😅

…om/CisterMoke/qiskit-terra into issue6772/dict_support_for_aux_ops

mrossinek

LGTM now! Thanks, @CisterMoke!

Cryoris

LGTM thanks for the updates 👍🏻

…iskit#6870) * Switch type from list to dict for aux_operators * Restored List support for aux_operators * Fixed a typo and two conditional checks * Added new unittest for NumPyMinimumEigenSolver and fixed lint issues * Added VQE unittest for testing aux_operators dictionaries * Updated aux_operator_eigenvalues type hint * Minor fix regarding VQE aux_operators * Update VQE and NumpyMinimumEigensolver unittests * Added a releasenote * Updated ListOrDict typehint * Remove unused imports Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

…6772) (Qiskit/qiskit#6870) * Switch type from list to dict for aux_operators * Restored List support for aux_operators * Fixed a typo and two conditional checks * Added new unittest for NumPyMinimumEigenSolver and fixed lint issues * Added VQE unittest for testing aux_operators dictionaries * Updated aux_operator_eigenvalues type hint * Minor fix regarding VQE aux_operators * Update VQE and NumpyMinimumEigensolver unittests * Added a releasenote * Updated ListOrDict typehint * Remove unused imports Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Switch type from list to dict for aux_operators

ef51d8b

CisterMoke requested review from manoelmarques, woodsp-ibm and a team as code owners August 4, 2021 23:59

CisterMoke added 2 commits August 7, 2021 13:40

Restored List support for aux_operators

23d0b31

Fixed a typo and two conditional checks

11ef00e

mrossinek reviewed Aug 9, 2021

View reviewed changes

Added new unittest for NumPyMinimumEigenSolver and fixed lint issues

7eb96e1

CisterMoke commented Aug 10, 2021

View reviewed changes

mrossinek reviewed Aug 11, 2021

View reviewed changes

Added VQE unittest for testing aux_operators dictionaries

9be9006

CisterMoke changed the title ~~[WIP] Support Dict[str, OperatorBase] for aux_operators (fix #6772)~~ Support Dict[str, OperatorBase] for aux_operators (fix #6772) Aug 17, 2021

mrossinek reviewed Aug 20, 2021

View reviewed changes

Updated aux_operator_eigenvalues type hint

4e09082

mrossinek previously approved these changes Aug 30, 2021

View reviewed changes

woodsp-ibm reviewed Aug 30, 2021

View reviewed changes

qiskit/algorithms/minimum_eigen_solvers/vqe.py Outdated Show resolved Hide resolved

qiskit/algorithms/minimum_eigen_solvers/vqe.py Outdated Show resolved Hide resolved

qiskit/algorithms/minimum_eigen_solvers/vqe.py Outdated Show resolved Hide resolved

Minor fix regarding VQE aux_operators

ccf1dc5

CisterMoke dismissed mrossinek’s stale review via ccf1dc5 August 30, 2021 22:42

Update VQE and NumpyMinimumEigensolver unittests

6824351

CisterMoke requested a review from woodsp-ibm August 31, 2021 22:40

mrossinek previously approved these changes Sep 6, 2021

View reviewed changes

woodsp-ibm previously approved these changes Sep 8, 2021

View reviewed changes

woodsp-ibm added the Changelog: New Feature Include in the "Added" section of the changelog label Sep 8, 2021

CisterMoke dismissed mrossinek’s stale review via 23cfa63 September 9, 2021 23:32

mrossinek previously approved these changes Sep 10, 2021

View reviewed changes

CisterMoke commented Sep 14, 2021

View reviewed changes

Merge branch 'main' into issue6772/dict_support_for_aux_ops

98a127f

CisterMoke dismissed mrossinek’s stale review via 98a127f September 21, 2021 19:06

mrossinek previously approved these changes Sep 22, 2021

View reviewed changes

woodsp-ibm added the mod: algorithms Related to the Algorithms module label Sep 22, 2021

woodsp-ibm previously approved these changes Sep 24, 2021

View reviewed changes

Merge branch 'main' into issue6772/dict_support_for_aux_ops

e5dfae3

Cryoris reviewed Oct 6, 2021

View reviewed changes

CisterMoke added 2 commits October 13, 2021 22:13

Updated ListOrDict typehint

68895c9

Merge branch 'issue6772/dict_support_for_aux_ops' of https://github.c…

808a193

…om/CisterMoke/qiskit-terra into issue6772/dict_support_for_aux_ops

CisterMoke dismissed stale reviews from woodsp-ibm and mrossinek via 808a193 October 13, 2021 20:15

CisterMoke added 3 commits October 13, 2021 22:23

Merge branch 'main' into issue6772/dict_support_for_aux_ops

fed1a7d

Remove unused imports

2b3a6d2

Merge branch 'issue6772/dict_support_for_aux_ops' of https://github.c…

c4b4679

…om/CisterMoke/qiskit-terra into issue6772/dict_support_for_aux_ops

mrossinek approved these changes Oct 14, 2021

View reviewed changes

Cryoris approved these changes Oct 14, 2021

View reviewed changes

Cryoris added the automerge label Oct 14, 2021

Merge branch 'main' into issue6772/dict_support_for_aux_ops

32e608b

mergify bot merged commit c1f960a into Qiskit:main Oct 14, 2021

This was referenced Oct 14, 2021

Leverage Dict-based aux_operators qiskit-community/qiskit-nature#396

Closed

API Regression: VQE aux_op_eigenvalues return type #7142

Closed

mrossinek mentioned this pull request Oct 22, 2021

Dict-based aux_operators qiskit-community/qiskit-nature#406

Merged

kdk added this to the 0.19 milestone Nov 15, 2021

	ListOrDict = Union[List[Optional[T]], Dict[Any, T]]
	ListOrDict = Union[List[Optional[T]], Dict[str, T]]

		result.aux_operator_eigenvalues = aux_values[0]
		result.aux_operator_eigenvalues = aux_values

	def aux_operator_eigenvalues(self) -> Optional[ListOrDict[Union[float, complex]]]:
	def aux_operator_eigenvalues(self) -> Optional[ListOrDict[complex]]:

		@@ -517,7 +532,7 @@ def compute_minimum_eigenvalue(

		if aux_operators is not None:
		aux_values = self._eval_aux_ops(opt_params, aux_operators, expectation=expectation)

Support Dict[str, OperatorBase] for aux_operators (fix #6772) #6870

Support Dict[str, OperatorBase] for aux_operators (fix #6772) #6870

Conversation

CisterMoke commented Aug 4, 2021 • edited Loading

Summary

Changelog

CLAassistant commented Aug 4, 2021 • edited Loading

woodsp-ibm commented Aug 5, 2021

CisterMoke commented Aug 5, 2021

mrossinek 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CisterMoke commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrossinek 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

woodsp-ibm Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CisterMoke commented Aug 11, 2021

mrossinek 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

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

mrossinek left a comment

Choose a reason for hiding this comment

mrossinek left a comment

Choose a reason for hiding this comment

woodsp-ibm left a comment

Choose a reason for hiding this comment

woodsp-ibm commented Sep 8, 2021

mrossinek left a comment

Choose a reason for hiding this comment

woodsp-ibm commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrossinek left a comment

Choose a reason for hiding this comment

woodsp-ibm left a comment

Choose a reason for hiding this comment

mrossinek commented Sep 29, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrossinek left a comment

Choose a reason for hiding this comment

Cryoris left a comment

Choose a reason for hiding this comment

CisterMoke commented Aug 4, 2021 •

edited

Loading

CLAassistant commented Aug 4, 2021 •

edited

Loading

woodsp-ibm Aug 11, 2021 •

edited

Loading