Dict-based `aux_operators` #406

mrossinek · 2021-10-22T16:01:39Z

Summary

This leverages Qiskit/qiskit#6870 by adding support for dict-based aux_operators.
For now, we definitely need to support both ways of adding aux_operators but I suggest that we deprecate the list-based method. See upcoming comments below.

Comments

Note, that my implementation of #26 will require this functionality.

Closes #396

We need to properly deprecate the old function signature of `second_q_ops` before we can fully switch to dict-based aux operators. For this, I introduce a new keyword argument `return_list` which defaults to the old way of list-based aux operators. In the following commits I will add DeprecationWarnings announcing a change of this default as well as unittests to assert the correct behavior of the dict-based aux operators.

mrossinek

Some explanatory comments

mrossinek · 2021-10-22T16:04:08Z

qiskit_nature/converters/second_quantization/qubit_converter.py

 from qiskit_nature.mappers.second_quantization import QubitMapper

 from qiskit_nature.operators.second_quantization import SecondQuantizedOp

+from .utils import ListOrDict


I'm not yet sure about the location of this utility class.

Since the dictionaries need to be passed on down to min eig solvers in Terra, which also define a type, maybe the type should come from there somehow rather than having to be independently defined here too.

Right now, this ListOrDict class is simply a wrapper which simplifies the iteration over whatever original type the operator were in. I did this because otherwise a lot of code would be duplicated in the internals of the QubitConverter.
In the end of the converter, the objects get's unwrapped anyways, before continuing with the rest of the stack.

That said, I agree that if we had a utility class such as this one on the Terra level, we would not need to do the unwrapping and could simply use that type everywhere. However, introducing such a class on the Terra level + using it as the type of how to pass operators back and forth would require exposing this utility class as part of the public API. I am not sure whether that is something we (or the Terra devs) want..

mrossinek · 2021-10-22T16:04:44Z

qiskit_nature/converters/second_quantization/qubit_converter.py

+        qubit_ops: ListOrDict[PauliSumOp] = ListOrDict()
+        for name, second_q_op in iter(wrapped_second_q_ops):
+            qubit_ops[name] = self._map(second_q_op)


FYI: this is a good example show-casing the usage of the ListOrDict utility class. We avoid the duplication of many isinstance checks against list or dict

qiskit_nature/converters/second_quantization/utils/list_or_dict.py

qiskit_nature/properties/second_quantization/electronic/angular_momentum.py

mrossinek · 2021-10-22T16:08:43Z

qiskit_nature/results/eigenstate_result.py

@@ -44,7 +46,9 @@ def __init__(self) -> None:
                ]
            ]
        ] = None
-        self._aux_operator_eigenvalues: Optional[List[float]] = None
+        self._aux_operator_eigenvalues: Optional[
+            List[ListOrDictType[Tuple[complex, complex]]]


Note the fix of the return type aligned with Qiskit/qiskit#7144

test/test_end2end_with_vqe.py

mrossinek · 2021-10-22T16:12:46Z

qiskit_nature/properties/second_quantization/electronic/angular_momentum.py

@@ -102,7 +103,7 @@ def from_legacy_driver_result(cls, result: LegacyDriverResult) -> "AngularMoment
            qmol.num_molecular_orbitals * 2,
        )

-    def second_q_ops(self) -> List[FermionicOp]:
+    def second_q_ops(self, return_list: bool = True) -> ListOrDictType[FermionicOp]:


I am suggesting this once on the first occurrence in the Files tab of this PR:

I would like to add a DeprecationWarning in the case of return_list = True, stating that in 3 months from now, the default will be changed to False, immediately implying that Qiskit Nature switches to dict-based operators as the default across the entire stack.
In 3 months from now, we can then add a new DeprecationWarning, stating that another 3 months later (i.e. 6 months from now), this keyword will be removed and, therefore, support for list-based aux operators will be dropped from the properties framework (I would argue to drop it from the entire stack but this can be discussed separately).

@woodsp-ibm @pbark I'm eager to hear your comments on this proposal 🙂

Since there is a flag, that the user could switch over to dicts to immediately go the future I guess its possibly to do this in one deprecation move rather than two. Earlier in one of my comments I noted about perhaps a single global control flag - having it in one place seemed easier to change behavior without changing the overall API. Maybe that could emit a single warning at some point if its set to list behavior rather than dict. Another thought, about the 'main' operator - basically this is done by having some known name on the problem right that represents the key for the main operator - what is in index 0 currently of the list. I did not look too closely at your ListDict class but was wondering if it could be done via that instead so its more self-contained. Like having a method to add main operator and then one for getting it so you can know the key or you can do ops.main_operator for instance.

I was also playing around with self-containing the name of the main operator on my ListOrDict utility. However, this runs into integrates that class very tightly into our stack and makes it a core type rather than a simple utility for iterating our operators. I noted in a reply above, that this would work better if we were to extract such a class into Terra because otherwise we run into issues with the interface to them.

For now, I would consider keeping it as is, unless you believe that the Terra devs are open to such a change. (Note, that it would expose the ListOrDict class as the expected type for all aux_operators in the public API)

As far as Terra is concerned there the main operator and the other operators that we wish to measure are separate - it is only in Nature where they were combined into one list/dict. I didn't look too much at the class - since as you say it needs to be unpacked/packed across the interface I guess it remains in Nature as such. The ListOrDict type however is common even if this class ends up being restricted to Nature.

Yes I see. So you would keep it self-contained to Nature but would still prefer it to be more integrated with the stack?

Since the intent is to eliminate List support I'm not sure an integration makes sense. The basic type, given whatever packing/upacking we do, needs to be consistent however we achieve that. The utility class I guess is a temporary artifact for Nature for the transition to dict only right - I will admit I did not look at it in detail.

qiskit_nature/problems/second_quantization/electronic/electronic_structure_problem.py

qiskit_nature/algorithms/excited_states_solvers/excited_states_eigensolver.py

woodsp-ibm · 2021-11-07T19:08:01Z

qiskit_nature/__init__.py

 from .version import __version__
 from .exceptions import QiskitNatureError, UnsupportMethodError

+# pylint: disable=invalid-name
+T = TypeVar("T")
+ListOrDictType = Union[List[Optional[T]], Dict[Union[int, str], T]]


Do you want to do the dict so its different than the minimum eigensolver - i.e it defines keys as just 'str'? The reason was to ensure it can be serialized. So if you define int here there arguably be an impedance mismatch when going down the stack whereby perhaps int will not behave as expected - I'm thinking here more around the runtime and remoting these.

Yeh you're right. Duplicating this is probably not a good way to go. Unfortunately, Qiskit Terra does not have one ground truth. Instead they have two definitions of ListOrDict in qiskit.algorithms.minimum_eigen_solvers and in qiskit.algorithms.eigen_solvers. While they are currently identical, this is rather error prone and could probably be improved.

qiskit_nature/algorithms/excited_states_solvers/excited_states_eigensolver.py

woodsp-ibm · 2021-11-07T19:32:09Z

qiskit_nature/converters/second_quantization/qubit_converter.py

 from qiskit_nature.mappers.second_quantization import QubitMapper

 from qiskit_nature.operators.second_quantization import SecondQuantizedOp

+from .utils import ListOrDict


Since the dictionaries need to be passed on down to min eig solvers in Terra, which also define a type, maybe the type should come from there somehow rather than having to be independently defined here too.

qiskit_nature/converters/second_quantization/qubit_converter.py

qiskit_nature/problems/second_quantization/electronic/electronic_structure_problem.py

woodsp-ibm · 2021-11-07T19:58:36Z

qiskit_nature/properties/second_quantization/electronic/angular_momentum.py

@@ -102,7 +103,7 @@ def from_legacy_driver_result(cls, result: LegacyDriverResult) -> "AngularMoment
            qmol.num_molecular_orbitals * 2,
        )

-    def second_q_ops(self) -> List[FermionicOp]:
+    def second_q_ops(self, return_list: bool = True) -> ListOrDictType[FermionicOp]:


Since there is a flag, that the user could switch over to dicts to immediately go the future I guess its possibly to do this in one deprecation move rather than two. Earlier in one of my comments I noted about perhaps a single global control flag - having it in one place seemed easier to change behavior without changing the overall API. Maybe that could emit a single warning at some point if its set to list behavior rather than dict. Another thought, about the 'main' operator - basically this is done by having some known name on the problem right that represents the key for the main operator - what is in index 0 currently of the list. I did not look too closely at your ListDict class but was wondering if it could be done via that instead so its more self-contained. Like having a method to add main operator and then one for getting it so you can know the key or you can do ops.main_operator for instance.

This property should only ever be set during construction of a certain problem type. Removing the setter ensures this scenario.

coveralls · 2021-11-22T09:18:48Z

Pull Request Test Coverage Report for Build 1516462477

237 of 298 (79.53%) changed or added relevant lines in 25 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.2%) to 85.342%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit_nature/converters/second_quantization/utils/list_or_dict.py	17	18	94.44%
qiskit_nature/properties/second_quantization/vibrational/occupied_modals.py	8	9	88.89%
qiskit_nature/properties/second_quantization/vibrational/vibrational_energy.py	4	5	80.0%
qiskit_nature/algorithms/excited_states_solvers/qeom.py	5	7	71.43%
qiskit_nature/properties/second_quantization/vibrational/vibrational_structure_driver_result.py	10	14	71.43%
qiskit_nature/converters/second_quantization/qubit_converter.py	30	39	76.92%
qiskit_nature/algorithms/ground_state_solvers/ground_state_eigensolver.py	11	22	50.0%
qiskit_nature/algorithms/excited_states_solvers/excited_states_eigensolver.py	7	23	30.43%
qiskit_nature/algorithms/ground_state_solvers/adapt_vqe.py	6	22	27.27%

Files with Coverage Reduction	New Missed Lines	%
qiskit_nature/properties/second_quantization/vibrational/vibrational_structure_driver_result.py	1	87.8%
qiskit_nature/drivers/psi4d/psi4driver.py	3	85.47%

Totals
Change from base Build 1516460021:	-0.2%
Covered Lines:	9671
Relevant Lines:	11332

💛 - Coveralls

mrossinek

@woodsp-ibm shared some more comments with me offline:

aux_operators are not being handled at all in QEOM. Arguably we should at least pass them on to the GSC
name conflicts for dict-based aux_operators provided by the user are not being handled at all, either. As things are currently, the user-provided operators would overwrite the internally created ones. I will add a guard against such name collisions and raise a warning. For the time being that should be sufficient (obviously it being documented, too)

* [WIP] naive migration to dict-based aux_operators * [WIP] extract ListOrDict logic into class * Revert ListOrDict integration We need to properly deprecate the old function signature of `second_q_ops` before we can fully switch to dict-based aux operators. For this, I introduce a new keyword argument `return_list` which defaults to the old way of list-based aux operators. In the following commits I will add DeprecationWarnings announcing a change of this default as well as unittests to assert the correct behavior of the dict-based aux operators. * Add basic unittest for dict-based aux ops * Refactor * Extend aux_operators public extension to support dict too * Fix lint * Revert some unnecessary changes * Update docstrings * Fix spell * Remove unused import * Fix lint * Reuse ListOrDict-type alias from Terra * Remove BaseProblem.main_property_name setter This property should only ever be set during construction of a certain problem type. Removing the setter ensures this scenario. * Improve commutation debug message * Extract new aux_operators interface into global setting * Run black * Add DeprecationWarning for list-based aux_operators * Log warning instead of raising it * Raise error upon aux_operator name clash * Evaluate aux_operators at ground state during QEOM Co-authored-by: Manoel Marques <[email protected]>

mrossinek added 7 commits October 15, 2021 14:53

[WIP] naive migration to dict-based aux_operators

5763cb7

[WIP] extract ListOrDict logic into class

ef8ca14

Add basic unittest for dict-based aux ops

f1852a1

Refactor

9fa525c

Extend aux_operators public extension to support dict too

e8a8f97

Fix lint

b520b1e

mrossinek added Changelog: API Change type: enhancement labels Oct 22, 2021

mrossinek commented Oct 22, 2021

View reviewed changes

Revert some unnecessary changes

ef06a84

mrossinek requested review from woodsp-ibm and pbark October 22, 2021 16:17

mrossinek and others added 5 commits October 26, 2021 08:31

Update docstrings

b9904ea

Fix spell

50fc492

Remove unused import

cb4ff7f

Merge branch 'main' into dict-based-aux-operators

bffba0f

Merge branch 'main' into dict-based-aux-operators

49d116d

woodsp-ibm reviewed Nov 7, 2021

View reviewed changes

mrossinek added 10 commits November 8, 2021 10:57

Merge branch 'main' into dict-based-aux-operators

250f3df

Fix lint

6448811

Reuse ListOrDict-type alias from Terra

702812a

Remove BaseProblem.main_property_name setter

3a518f4

This property should only ever be set during construction of a certain problem type. Removing the setter ensures this scenario.

Improve commutation debug message

a0cc750

Extract new aux_operators interface into global setting

1c05bd4

Run black

49870c4

Add DeprecationWarning for list-based aux_operators

c45c6de

Log warning instead of raising it

c344832

Merge branch 'main' into dict-based-aux-operators

f65105f

mrossinek marked this pull request as ready for review November 8, 2021 15:36

mrossinek requested review from manoelmarques and stefan-woerner as code owners November 8, 2021 15:36

mrossinek requested a review from woodsp-ibm November 8, 2021 15:36

This was referenced Nov 8, 2021

Simplify logging configuration #419

Closed

Allow custom Property integration #312

Closed

mrossinek added 3 commits November 10, 2021 17:17

Merge branch 'main' into dict-based-aux-operators

057c4ce

Merge branch 'main' into dict-based-aux-operators

93632ef

Merge branch 'main' into dict-based-aux-operators

ab3e4d6

manoelmarques and others added 2 commits November 23, 2021 10:27

Merge main and fix conflicts

e21bfa7

Merge branch 'main' into dict-based-aux-operators

985e2be

manoelmarques previously approved these changes Nov 23, 2021

View reviewed changes

mrossinek commented Nov 24, 2021

View reviewed changes

manoelmarques and others added 4 commits November 24, 2021 10:12

Merge branch 'main' into dict-based-aux-operators

a5de212

Merge branch 'main' into dict-based-aux-operators

646f0ea

Raise error upon aux_operator name clash

374e30c

Evaluate aux_operators at ground state during QEOM

58ab2ed

mrossinek dismissed manoelmarques’s stale review via 58ab2ed November 25, 2021 13:00

mrossinek requested a review from manoelmarques November 25, 2021 13:28

manoelmarques added 3 commits November 25, 2021 12:01

Merge branch 'main' into dict-based-aux-operators

1f36750

Merge branch 'main' into dict-based-aux-operators

727460f

Merge branch 'main' into dict-based-aux-operators

bd3368b

manoelmarques approved these changes Nov 29, 2021

View reviewed changes

mrossinek merged commit 80a23f4 into qiskit-community:main Dec 1, 2021

mrossinek deleted the dict-based-aux-operators branch December 1, 2021 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dict-based `aux_operators` #406

Dict-based `aux_operators` #406

mrossinek commented Oct 22, 2021 •

edited

Loading

mrossinek left a comment

mrossinek Oct 22, 2021

woodsp-ibm Nov 7, 2021

mrossinek Nov 8, 2021

mrossinek Oct 22, 2021

mrossinek Oct 22, 2021

mrossinek Oct 22, 2021

woodsp-ibm Nov 7, 2021

mrossinek Nov 8, 2021

woodsp-ibm Nov 8, 2021

mrossinek Nov 8, 2021

woodsp-ibm Nov 8, 2021

woodsp-ibm Nov 7, 2021

mrossinek Nov 8, 2021

woodsp-ibm Nov 7, 2021

woodsp-ibm Nov 7, 2021

coveralls commented Nov 22, 2021 •

edited

Loading

mrossinek left a comment

Dict-based aux_operators #406

Dict-based aux_operators #406

Conversation

mrossinek commented Oct 22, 2021 • edited Loading

Summary

Comments

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

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

coveralls commented Nov 22, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1516462477

💛 - Coveralls

mrossinek left a comment

Choose a reason for hiding this comment

Dict-based `aux_operators` #406

Dict-based `aux_operators` #406

mrossinek commented Oct 22, 2021 •

edited

Loading

coveralls commented Nov 22, 2021 •

edited

Loading