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.

Sign up for GitHub

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

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

Merged
merged 17 commits into from
Oct 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ef51d8b
Switch type from list to dict for aux_operators
CisterMoke Aug 4, 2021
23d0b31
Restored List support for aux_operators
CisterMoke Aug 7, 2021
11ef00e
Fixed a typo and two conditional checks
CisterMoke Aug 7, 2021
7eb96e1
Added new unittest for NumPyMinimumEigenSolver and fixed lint issues
CisterMoke Aug 10, 2021
9be9006
Added VQE unittest for testing aux_operators dictionaries
CisterMoke Aug 14, 2021
4e09082
Updated aux_operator_eigenvalues type hint
CisterMoke Aug 29, 2021
ccf1dc5
Minor fix regarding VQE aux_operators
CisterMoke Aug 30, 2021
6824351
Update VQE and NumpyMinimumEigensolver unittests
CisterMoke Aug 31, 2021
23cfa63
Added a releasenote
CisterMoke Sep 9, 2021
98a127f
Merge branch 'main' into issue6772/dict_support_for_aux_ops
CisterMoke Sep 21, 2021
e5dfae3
Merge branch 'main' into issue6772/dict_support_for_aux_ops
woodsp-ibm Sep 30, 2021
68895c9
Updated ListOrDict typehint
CisterMoke Oct 13, 2021
808a193
Merge branch 'issue6772/dict_support_for_aux_ops' of https://github.c…
CisterMoke Oct 13, 2021
fed1a7d
Merge branch 'main' into issue6772/dict_support_for_aux_ops
CisterMoke Oct 13, 2021
2b3a6d2
Remove unused imports
CisterMoke Oct 13, 2021
c4b4679
Merge branch 'issue6772/dict_support_for_aux_ops' of https://github.c…
CisterMoke Oct 13, 2021
32e608b
Merge branch 'main' into issue6772/dict_support_for_aux_ops
mergify[bot] Oct 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions qiskit/algorithms/eigen_solvers/eigen_solver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@
"""The Eigensolver interface"""

from abc import ABC, abstractmethod
from typing import List, Optional
from typing import Dict, Optional, Any, List, Union, TypeVar

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]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



class Eigensolver(ABC):
"""The Eigensolver Interface.
Expand All @@ -30,7 +34,7 @@ class Eigensolver(ABC):

@abstractmethod
def compute_eigenvalues(
self, operator: OperatorBase, aux_operators: Optional[List[Optional[OperatorBase]]] = None
self, operator: OperatorBase, aux_operators: Optional[ListOrDict[OperatorBase]] = None
) -> "EigensolverResult":
"""
Computes eigenvalues. Operator and aux_operators can be supplied here and
Expand Down Expand Up @@ -90,11 +94,11 @@ def eigenstates(self, value: np.ndarray) -> None:
self._eigenstates = value

@property
def aux_operator_eigenvalues(self) -> Optional[np.ndarray]:
def aux_operator_eigenvalues(self) -> Optional[List[ListOrDict[complex]]]:
"""return aux operator eigen values"""
return self._aux_operator_eigenvalues

@aux_operator_eigenvalues.setter
def aux_operator_eigenvalues(self, value: np.ndarray) -> None:
def aux_operator_eigenvalues(self, value: List[ListOrDict[complex]]) -> None:
"""set aux operator eigen values"""
self._aux_operator_eigenvalues = value
43 changes: 28 additions & 15 deletions qiskit/algorithms/eigen_solvers/numpy_eigen_solver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

from qiskit.opflow import OperatorBase, I, StateFn, ListOp
from qiskit.utils.validation import validate_min
from .eigen_solver import Eigensolver, EigensolverResult
from .eigen_solver import Eigensolver, EigensolverResult, ListOrDict
from ..exceptions import AlgorithmError

logger = logging.getLogger(__name__)
Expand All @@ -46,7 +46,7 @@ def __init__(
self,
k: int = 1,
filter_criterion: Callable[
[Union[List, np.ndarray], float, Optional[List[float]]], bool
[Union[List, np.ndarray], float, Optional[ListOrDict[float]]], bool
] = None,
) -> None:
"""
Expand Down Expand Up @@ -84,15 +84,15 @@ def k(self, k: int) -> None:
@property
def filter_criterion(
self,
) -> Optional[Callable[[Union[List, np.ndarray], float, Optional[List[float]]], bool]]:
) -> Optional[Callable[[Union[List, np.ndarray], float, Optional[ListOrDict[float]]], bool]]:
"""returns the filter criterion if set"""
return self._filter_criterion

@filter_criterion.setter
def filter_criterion(
self,
filter_criterion: Optional[
Callable[[Union[List, np.ndarray], float, Optional[List[float]]], bool]
Callable[[Union[List, np.ndarray], float, Optional[ListOrDict[float]]], bool]
],
) -> None:
"""set the filter criterion"""
Expand Down Expand Up @@ -141,7 +141,7 @@ def _get_ground_state_energy(self, operator: OperatorBase) -> None:
self._solve(operator)

def _get_energies(
self, operator: OperatorBase, aux_operators: Optional[List[OperatorBase]]
self, operator: OperatorBase, aux_operators: Optional[ListOrDict[OperatorBase]]
) -> None:
if self._ret.eigenvalues is None or self._ret.eigenstates is None:
self._solve(operator)
Expand All @@ -156,12 +156,19 @@ def _get_energies(

@staticmethod
def _eval_aux_operators(
aux_operators: List[OperatorBase], wavefn, threshold: float = 1e-12
) -> np.ndarray:
values = [] # type: List[Tuple[float, int]]
for operator in aux_operators:
aux_operators: ListOrDict[OperatorBase], wavefn, threshold: float = 1e-12
) -> ListOrDict[Tuple[float, int]]:

# As a list, aux_operators can contain None operators for which None values are returned.
# As a dict, the None operators in aux_operators have been dropped in compute_eigenvalues.
if isinstance(aux_operators, list):
values = [None] * len(aux_operators) # type: ListOrDict[Tuple[float, int]]
key_op_iterator = enumerate(aux_operators)
else:
values = {}
key_op_iterator = aux_operators.items()
for key, operator in key_op_iterator:
if operator is None:
values.append(None)
continue
value = 0.0
if operator.coeff != 0:
Expand All @@ -174,22 +181,28 @@ def _eval_aux_operators(
else:
value = StateFn(operator, is_measurement=True).eval(wavefn)
value = value.real if abs(value.real) > threshold else 0.0
values.append((value, 0))
return np.array(values, dtype=object)
values[key] = (value, 0)
return values

def compute_eigenvalues(
self, operator: OperatorBase, aux_operators: Optional[List[Optional[OperatorBase]]] = None
self, operator: OperatorBase, aux_operators: Optional[ListOrDict[OperatorBase]] = None
) -> EigensolverResult:
super().compute_eigenvalues(operator, aux_operators)

if operator is None:
raise AlgorithmError("Operator was never provided")

self._check_set_k(operator)
if aux_operators:
zero_op = I.tensorpower(operator.num_qubits) * 0.0
zero_op = I.tensorpower(operator.num_qubits) * 0.0
if isinstance(aux_operators, list) and len(aux_operators) > 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: zero_op if op == 0 else op # Convert zero values to zero operators
for key, op in aux_operators.items()
if op is not None # Discard None values
}
else:
aux_operators = None

Expand Down
12 changes: 8 additions & 4 deletions qiskit/algorithms/minimum_eigen_solvers/minimum_eigen_solver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@
"""The Minimum Eigensolver interface"""

from abc import ABC, abstractmethod
from typing import List, Optional
from typing import Dict, Optional, Any, List, Union, TypeVar

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]]


class MinimumEigensolver(ABC):
"""The Minimum Eigensolver Interface.
Expand All @@ -30,7 +34,7 @@ class MinimumEigensolver(ABC):

@abstractmethod
def compute_minimum_eigenvalue(
self, operator: OperatorBase, aux_operators: Optional[List[Optional[OperatorBase]]] = None
self, operator: OperatorBase, aux_operators: Optional[ListOrDict[OperatorBase]] = None
) -> "MinimumEigensolverResult":
"""
Computes minimum eigenvalue. Operator and aux_operators can be supplied here and
Expand Down Expand Up @@ -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[complex]]:
"""return aux operator eigen values"""
return self._aux_operator_eigenvalues

@aux_operator_eigenvalues.setter
def aux_operator_eigenvalues(self, value: np.ndarray) -> None:
def aux_operator_eigenvalues(self, value: ListOrDict[complex]) -> None:
"""set aux operator eigen values"""
self._aux_operator_eigenvalues = value
10 changes: 5 additions & 5 deletions qiskit/algorithms/minimum_eigen_solvers/numpy_minimum_eigen_solver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

from qiskit.opflow import OperatorBase
from ..eigen_solvers.numpy_eigen_solver import NumPyEigensolver
from .minimum_eigen_solver import MinimumEigensolver, MinimumEigensolverResult
from .minimum_eigen_solver import MinimumEigensolver, MinimumEigensolverResult, ListOrDict

logger = logging.getLogger(__name__)

Expand All @@ -31,7 +31,7 @@ class NumPyMinimumEigensolver(MinimumEigensolver):
def __init__(
self,
filter_criterion: Callable[
[Union[List, np.ndarray], float, Optional[List[float]]], bool
[Union[List, np.ndarray], float, Optional[ListOrDict[float]]], bool
] = None,
) -> None:
"""
Expand All @@ -49,15 +49,15 @@ def __init__(
@property
def filter_criterion(
self,
) -> Optional[Callable[[Union[List, np.ndarray], float, Optional[List[float]]], bool]]:
) -> Optional[Callable[[Union[List, np.ndarray], float, Optional[ListOrDict[float]]], bool]]:
"""returns the filter criterion if set"""
return self._ces.filter_criterion

@filter_criterion.setter
def filter_criterion(
self,
filter_criterion: Optional[
Callable[[Union[List, np.ndarray], float, Optional[List[float]]], bool]
Callable[[Union[List, np.ndarray], float, Optional[ListOrDict[float]]], bool]
],
) -> None:
"""set the filter criterion"""
Expand All @@ -68,7 +68,7 @@ def supports_aux_operators(cls) -> bool:
return NumPyEigensolver.supports_aux_operators()

def compute_minimum_eigenvalue(
self, operator: OperatorBase, aux_operators: Optional[List[Optional[OperatorBase]]] = None
self, operator: OperatorBase, aux_operators: Optional[ListOrDict[OperatorBase]] = None
) -> MinimumEigensolverResult:
super().compute_minimum_eigenvalue(operator, aux_operators)
result_ces = self._ces.compute_eigenvalues(operator, aux_operators)
Expand Down
65 changes: 40 additions & 25 deletions qiskit/algorithms/minimum_eigen_solvers/vqe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
from qiskit.utils import QuantumInstance, algorithm_globals
from ..optimizers import Optimizer, SLSQP
from ..variational_algorithm import VariationalAlgorithm, VariationalResult
from .minimum_eigen_solver import MinimumEigensolver, MinimumEigensolverResult
from .minimum_eigen_solver import MinimumEigensolver, MinimumEigensolverResult, ListOrDict
from ..exceptions import AlgorithmError

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -411,32 +411,42 @@ def supports_aux_operators(cls) -> bool:
def _eval_aux_ops(
self,
parameters: np.ndarray,
aux_operators: List[OperatorBase],
aux_operators: ListOrDict[OperatorBase],
expectation: ExpectationBase,
threshold: float = 1e-12,
) -> np.ndarray:
) -> ListOrDict[complex]:
# 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()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's very good to know! 👍

else:
list_op = ListOp(aux_operators)

aux_op_meas = expectation.convert(StateFn(list_op, is_measurement=True))
aux_op_expect = aux_op_meas.compose(CircuitStateFn(self.ansatz.bind_parameters(parameters)))
values = np.real(sampler.convert(aux_op_expect).eval())

# Discard values below threshold
aux_op_results = values * (np.abs(values) > threshold)
# Deal with the aux_op behavior where there can be Nones or Zero qubit Paulis in the list
_aux_op_nones = [op is None for op in aux_operators]
aux_operator_eigenvalues = [
None if is_none else [result]
for (is_none, result) in zip(_aux_op_nones, aux_op_results)
]
# As this has mixed types, since it can included None, it needs to explicitly pass object
# data type to avoid numpy 1.19 warning message about implicit conversion being deprecated
aux_operator_eigenvalues = np.array([aux_operator_eigenvalues], dtype=object)

# Return None eigenvalues for None operators if aux_operators is a list.
# None operators are already dropped in compute_minimum_eigenvalue if aux_operators is a dict.
if isinstance(aux_operators, list):
aux_operator_eigenvalues = [None] * len(aux_operators)
key_value_iterator = enumerate(aux_op_results)
else:
aux_operator_eigenvalues = {}
key_value_iterator = zip(aux_operators.keys(), aux_op_results)

for key, value in key_value_iterator:
if aux_operators[key] is not None:
aux_operator_eigenvalues[key] = value

return aux_operator_eigenvalues

def compute_minimum_eigenvalue(
self, operator: OperatorBase, aux_operators: Optional[List[Optional[OperatorBase]]] = None
self, operator: OperatorBase, aux_operators: Optional[ListOrDict[OperatorBase]] = None
) -> MinimumEigensolverResult:
super().compute_minimum_eigenvalue(operator, aux_operators)

Expand All @@ -454,19 +464,24 @@ def compute_minimum_eigenvalue(
initial_point = _validate_initial_point(self.initial_point, self.ansatz)

bounds = _validate_bounds(self.ansatz)

# We need to handle the array entries being Optional i.e. having value None
# We need to handle the array entries being zero or Optional i.e. having value None
if aux_operators:
zero_op = I.tensorpower(operator.num_qubits) * 0.0
converted = []
for op in aux_operators:
if op is None:
converted.append(zero_op)
else:
converted.append(op)

# 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 converted]
# Convert the None and zero values when aux_operators is a list.
# Drop None and convert zero values when aux_operators is a dict.
if isinstance(aux_operators, list):
key_op_iterator = enumerate(aux_operators)
converted = [zero_op] * len(aux_operators)
else:
key_op_iterator = aux_operators.items()
converted = {}
for key, op in key_op_iterator:
if op is not None:
converted[key] = zero_op if op == 0 else op

aux_operators = converted

else:
aux_operators = None

Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

result.aux_operator_eigenvalues = aux_values[0]
result.aux_operator_eigenvalues = aux_values
Comment on lines -535 to +550
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@woodsp-ibm woodsp-ibm Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return result

Expand Down
Loading