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.

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
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
8 changes: 6 additions & 2 deletions qiskit/algorithms/eigen_solvers/eigen_solver.py
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')
ListOrDict = Union[List[Optional[T]], Dict[Any, T]]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.



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
39 changes: 24 additions & 15 deletions qiskit/algorithms/eigen_solvers/numpy_eigen_solver.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
"""The Eigensolver algorithm."""

import logging
from typing import List, Optional, Union, Tuple, Callable
from typing import List, Optional, Union, Tuple, Callable, Dict, Any, TypeVar

import numpy as np
from scipy import sparse as scisparse

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

# If aux_operators is a list, it can contain None operators for which None values are returned.
# If aux_operators is a dict, the None 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,24 @@ 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:
if isinstance(aux_operators, list) and len(aux_operators) > 0:
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}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, the not should not be there

else:
aux_operators = None

Expand Down
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')
ListOrDict = Union[List[Optional[T]], Dict[Any, T]]
Copy link
Member

Choose a reason for hiding this comment

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

Just as before:

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

Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to above.



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
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@

"""The Numpy Minimum Eigensolver algorithm."""

from typing import List, Optional, Union, Callable
from typing import List, Optional, Union, Callable, Dict
import logging
import numpy as np

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
61 changes: 41 additions & 20 deletions qiskit/algorithms/minimum_eigen_solvers/vqe.py
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,49 @@ 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:
# 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)
]

# Return None eigenvalues for None operators if aux_operators is a list.
# If aux_operators is a dict, the None operators should have been dropped in compute_minimum_eigenvalue
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
# # 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()
# }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

# 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 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 +471,23 @@ 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
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 operators when aux_operators is a list. Drop them is 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] = op

aux_operators = converted

else:
aux_operators = None

Expand Down Expand Up @@ -517,7 +538,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

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