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 1 commit
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
4 changes: 2 additions & 2 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 @@ -20,8 +20,8 @@
from ..algorithm_result import AlgorithmResult

# Introduced new type to maintain readability.
T = TypeVar('T')
ListOrDict = Union[List[Optional[T]], Dict[Any, T]]
_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):
Expand Down
8 changes: 4 additions & 4 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 @@ -13,7 +13,7 @@
"""The Eigensolver algorithm."""

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

import numpy as np
from scipy import sparse as scisparse
Expand Down Expand Up @@ -159,8 +159,8 @@ def _eval_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.
# 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)
Expand Down Expand Up @@ -198,7 +198,7 @@ def compute_eigenvalues(
# 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}
aux_operators = {key: op for key, op in aux_operators.items() if op}
else:
aux_operators = None

Expand Down
4 changes: 2 additions & 2 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 @@ -20,8 +20,8 @@
from ..algorithm_result import AlgorithmResult

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


class MinimumEigensolver(ABC):
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -12,7 +12,7 @@

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

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

Expand Down
5 changes: 3 additions & 2 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 @@ -431,7 +431,7 @@ def _eval_aux_ops(
aux_op_results = values * (np.abs(values) > threshold)

# 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
# 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)
Expand Down Expand Up @@ -475,7 +475,8 @@ def compute_minimum_eigenvalue(
if aux_operators:
zero_op = I.tensorpower(operator.num_qubits) * 0.0

# Convert the None operators when aux_operators is a list. Drop them is aux_operators is a dict.
# Convert the None operators when aux_operators is a list.
# Drop None operators when aux_operators is a dict.
if isinstance(aux_operators, list):
key_op_iterator = enumerate(aux_operators)
converted = [zero_op] * len(aux_operators)
Expand Down
50 changes: 43 additions & 7 deletions test/python/algorithms/test_numpy_minimum_eigen_solver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,15 @@ def setUp(self):

aux_op1 = PauliSumOp.from_list([("II", 2.0)])
aux_op2 = PauliSumOp.from_list([("II", 0.5), ("ZZ", 0.5), ("YY", 0.5), ("XX", -0.5)])
self.aux_ops = [aux_op1, aux_op2]
self.aux_ops_list = [aux_op1, aux_op2]
self.aux_ops_dict = {"aux_op1": aux_op1, "aux_op2": aux_op2}

def test_cme(self):
"""Basic test"""
algo = NumPyMinimumEigensolver()
result = algo.compute_minimum_eigenvalue(operator=self.qubit_op, aux_operators=self.aux_ops)
result = algo.compute_minimum_eigenvalue(
operator=self.qubit_op, aux_operators=self.aux_ops_list
)
self.assertAlmostEqual(result.eigenvalue, -1.85727503 + 0j)
self.assertEqual(len(result.aux_operator_eigenvalues), 2)
np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues[0], [2, 0])
Expand All @@ -60,7 +63,9 @@ def test_cme_reuse(self):
self.assertIsNone(result.aux_operator_eigenvalues)

# Add aux_operators and go again
result = algo.compute_minimum_eigenvalue(operator=self.qubit_op, aux_operators=self.aux_ops)
result = algo.compute_minimum_eigenvalue(
operator=self.qubit_op, aux_operators=self.aux_ops_list
)
self.assertAlmostEqual(result.eigenvalue, -1.85727503 + 0j)
self.assertEqual(len(result.aux_operator_eigenvalues), 2)
np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues[0], [2, 0])
Expand All @@ -72,14 +77,16 @@ def test_cme_reuse(self):
self.assertIsNone(result.aux_operator_eigenvalues)

# Set aux_operators and go again
result = algo.compute_minimum_eigenvalue(operator=self.qubit_op, aux_operators=self.aux_ops)
result = algo.compute_minimum_eigenvalue(
operator=self.qubit_op, aux_operators=self.aux_ops_list
)
self.assertAlmostEqual(result.eigenvalue, -1.85727503 + 0j)
self.assertEqual(len(result.aux_operator_eigenvalues), 2)
np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues[0], [2, 0])
np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues[1], [0, 0])

# Finally just set one of aux_operators and main operator, remove aux_operators
result = algo.compute_minimum_eigenvalue(operator=self.aux_ops[0], aux_operators=[])
result = algo.compute_minimum_eigenvalue(operator=self.aux_ops_list[0], aux_operators=[])
self.assertAlmostEqual(result.eigenvalue, 2 + 0j)
self.assertIsNone(result.aux_operator_eigenvalues)

Expand All @@ -92,7 +99,9 @@ def criterion(x, v, a_v):
return v >= -0.5

algo = NumPyMinimumEigensolver(filter_criterion=criterion)
result = algo.compute_minimum_eigenvalue(operator=self.qubit_op, aux_operators=self.aux_ops)
result = algo.compute_minimum_eigenvalue(
operator=self.qubit_op, aux_operators=self.aux_ops_list
)
self.assertAlmostEqual(result.eigenvalue, -0.22491125 + 0j)
self.assertEqual(len(result.aux_operator_eigenvalues), 2)
np.testing.assert_array_almost_equal(result.aux_operator_eigenvalues[0], [2, 0])
Expand All @@ -107,7 +116,9 @@ def criterion(x, v, a_v):
return False

algo = NumPyMinimumEigensolver(filter_criterion=criterion)
result = algo.compute_minimum_eigenvalue(operator=self.qubit_op, aux_operators=self.aux_ops)
result = algo.compute_minimum_eigenvalue(
operator=self.qubit_op, aux_operators=self.aux_ops_list
)
self.assertEqual(result.eigenvalue, None)
self.assertEqual(result.eigenstate, None)
self.assertEqual(result.aux_operator_eigenvalues, None)
Expand All @@ -119,6 +130,31 @@ def test_cme_1q(self, op):
result = algo.compute_minimum_eigenvalue(operator=op)
self.assertAlmostEqual(result.eigenvalue, -1)

def test_cme_aux_ops_dict(self):
"""Test dictionary compatibility of aux_operators"""
# Start with an empty dictionary
algo = NumPyMinimumEigensolver()
result = algo.compute_minimum_eigenvalue(operator=self.qubit_op, aux_operators={})
self.assertAlmostEqual(result.eigenvalue, -1.85727503 + 0j)
self.assertIsNone(result.aux_operator_eigenvalues)

# Add aux_operators dictionary and go again
result = algo.compute_minimum_eigenvalue(
operator=self.qubit_op, aux_operators=self.aux_ops_dict
)
self.assertAlmostEqual(result.eigenvalue, -1.85727503 + 0j)
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])

# Add None and zero operators and go again
extra_ops = {"None_op": None, "zero_op": 0, **self.aux_ops_dict}
result = algo.compute_minimum_eigenvalue(operator=self.qubit_op, aux_operators=extra_ops)
self.assertAlmostEqual(result.eigenvalue, -1.85727503 + 0j)
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])


Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

if __name__ == "__main__":
unittest.main()