Separate simplification of operators from SummedOp.add and fix bugs of add

qiskit/aqua/components/optimizers/aqgd.py

@@ -141,7 +141,7 @@ def converged(self, objval, n=2):
         if self._previous_loss is None:
             self._previous_loss = [objval + 2 * self._tol] * n
 
-        if all([absolute(objval - prev) < self._tol for prev in self._previous_loss]):
+        if all(absolute(objval - prev) < self._tol for prev in self._previous_loss):
             # converged
             return True
 

qiskit/aqua/operators/converters/abelian_grouper.py

@@ -12,7 +12,7 @@
 # copyright notice, and modified files need to carry a notice indicating
 # that they have been altered from the originals.
 
-""" AbelianGrouper Class """
+"""AbelianGrouper Class"""
 
 import itertools
 from collections import defaultdict
@@ -31,15 +31,16 @@
 
 
 class AbelianGrouper(ConverterBase):
-    """
-    The AbelianGrouper converts SummedOps into a sum of Abelian sums. Meaning,
-    it will traverse the Operator, and when it finds a SummedOp, it will evaluate which of the
-    summed sub-Operators commute with one another. It will then convert each of the groups of
+    """The AbelianGrouper converts SummedOps into a sum of Abelian sums.
+
+    Meaning, it will traverse the Operator, and when it finds a SummedOp, it will evaluate which of
+    the summed sub-Operators commute with one another. It will then convert each of the groups of
     commuting Operators into their own SummedOps, and return the sum-of-commuting-SummedOps.
     This is particularly useful for cases where mutually commuting groups can be handled
     similarly, as in the case of Pauli Expectations, where commuting Paulis have the same
     diagonalizing circuit rotation, or Pauli Evolutions, where commuting Paulis can be
-    diagonalized together. """
+    diagonalized together.
+    """
 
     def __init__(self, traverse: bool = True) -> None:
         """
@@ -50,7 +51,7 @@ def __init__(self, traverse: bool = True) -> None:
         self._traverse = traverse
 
     def convert(self, operator: OperatorBase) -> OperatorBase:
-        """ Check if operator is a SummedOp, in which case covert it into a sum of mutually
+        """Check if operator is a SummedOp, in which case covert it into a sum of mutually
         commuting sums, or if the Operator contains sub-Operators and ``traverse`` is True,
         attempt to convert any sub-Operators.
 
@@ -83,7 +84,7 @@ def convert(self, operator: OperatorBase) -> OperatorBase:
 
     @classmethod
     def group_subops(cls, list_op: ListOp, fast: bool = True, use_nx: bool = False) -> ListOp:
-        """ Given a ListOp, attempt to group into Abelian ListOps of the same type.
+        """Given a ListOp, attempt to group into Abelian ListOps of the same type.
 
         Args:
             list_op: The Operator to group into Abelian groups
@@ -124,8 +125,7 @@ def group_subops(cls, list_op: ListOp, fast: bool = True, use_nx: bool = False)
 
     @staticmethod
     def _commutation_graph(list_op: ListOp) -> List[Tuple[int, int]]:
-        """
-        Create edges (i, j) if i and j is not commutable.
+        """Create edges (i, j) if i and j are not commutable.
 
         Args:
             list_op: list_op
@@ -139,9 +139,10 @@ def _commutation_graph(list_op: ListOp) -> List[Tuple[int, int]]:
 
     @staticmethod
     def _commutation_graph_fast(list_op: ListOp) -> List[Tuple[int, int]]:
-        """
-        Create edges (i, j) if i and j is not commutable.
-        Note that this method is applicable to only PauliOps.
+        """Create edges (i, j) if i and j are not commutable.
+
+        Note:
+            This method is applicable to only PauliOps.
 
         Args:
             list_op: list_op
@@ -152,7 +153,7 @@ def _commutation_graph_fast(list_op: ListOp) -> List[Tuple[int, int]]:
         # convert a Pauli operator into int vector where {I: 0, X: 2, Y: 3, Z: 1}
         mat1 = np.array([op.primitive.z + 2 * op.primitive.x for op in list_op], dtype=np.int8)
         mat2 = mat1[:, None]
-        # i and j are commutable with TPB if mat3[i, j] is True
+        # mat3[i, j] is True if i and j are commutable with TPB
         mat3 = (((mat1 * mat2) * (mat1 - mat2)) == 0).all(axis=2)
         # return [(i, j) if mat3[i, j] is False and i < j]
         return zip(*np.where(np.triu(np.logical_not(mat3), k=1)))

qiskit/aqua/operators/list_ops/list_op.py

@@ -14,15 +14,15 @@
 
 """ ListOp Operator Class """
 
-from typing import List, Union, Optional, Callable, Iterator, Set, Dict
 from functools import reduce
+from typing import List, Union, Optional, Callable, Iterator, Set, Dict
+
 import numpy as np
 from scipy.sparse import spmatrix
 
 from qiskit.circuit import ParameterExpression
-
-from ..operator_base import OperatorBase
 from ..legacy.base_operator import LegacyBaseOperator
+from ..operator_base import OperatorBase
 
 
 class ListOp(OperatorBase):
@@ -158,7 +158,8 @@ def equals(self, other: OperatorBase) -> bool:
         if not isinstance(other, type(self)) or not len(self.oplist) == len(other.oplist):
             return False
         # Note, ordering matters here (i.e. different list orders will return False)
-        return all([op1 == op2 for op1, op2 in zip(self.oplist, other.oplist)])
+        return self.coeff == other.coeff and all(
+            op1 == op2 for op1, op2 in zip(self.oplist, other.oplist))
 
     # We need to do this because otherwise Numpy takes over scalar multiplication and wrecks it if
     # isinstance(scalar, np.number) - this started happening when we added __get_item__().
@@ -210,7 +211,7 @@ def to_matrix(self, massive: bool = False) -> np.ndarray:
             raise ValueError(
                 'to_matrix will return an exponentially large matrix, '
                 'in this case {0}x{0} elements.'
-                ' Set massive=True if you want to proceed.'.format(2**self.num_qubits))
+                ' Set massive=True if you want to proceed.'.format(2 ** self.num_qubits))
 
         # Combination function must be able to handle classical values
         return self.combo_fn([op.to_matrix() * self.coeff for op in self.oplist])
@@ -265,9 +266,9 @@ def eval(self,
                                       r'Listops.')
 
         evals = [(self.coeff * op).eval(front) for op in self.oplist]
-        if all([isinstance(op, OperatorBase) for op in evals]):
+        if all(isinstance(op, OperatorBase) for op in evals):
             return self.__class__(evals)
-        elif any([isinstance(op, OperatorBase) for op in evals]):
+        elif any(isinstance(op, OperatorBase) for op in evals):
             raise TypeError('Cannot handle mixed scalar and Operator eval results.')
         else:
             return self.combo_fn(evals)

qiskit/aqua/operators/list_ops/summed_op.py

@@ -14,17 +14,17 @@
 
 """ SummedOp Class """
 
-from typing import List, Union
-import copy
 from functools import reduce
+from typing import List, Union
+
 import numpy as np
 
 from qiskit.circuit import ParameterExpression
-
-from ..operator_base import OperatorBase
 from .list_op import ListOp
 from ..legacy.base_operator import LegacyBaseOperator
 from ..legacy.weighted_pauli_operator import WeightedPauliOperator
+from ..operator_base import OperatorBase
+from ..primitive_ops.primitive_op import PrimitiveOp
 
 
 class SummedOp(ListOp):
@@ -33,6 +33,7 @@ class SummedOp(ListOp):
     later. This class holds logic to indicate that the Operators in ``oplist`` are meant to
     be added together, and therefore if they reach a point in which they can be, such as after
     evaluation or conversion to matrices, they can be reduced by addition. """
+
     def __init__(self,
                  oplist: List[OperatorBase],
                  coeff: Union[int, float, complex, ParameterExpression] = 1.0,
@@ -57,18 +58,60 @@ def distributive(self) -> bool:
         return True
 
     def add(self, other: OperatorBase) -> OperatorBase:
+        """Return Operator addition of ``self`` and ``other``, overloaded by ``+``.
+
+        Note:
+            This appends ``other`` to ``self.oplist`` without checking ``other`` is already
+            included or not. If you want to simplify them, please use :meth:`simplify`.
+
+        Args:
+            other: An ``OperatorBase`` with the same number of qubits as self, and in the same
+                'Operator', 'State function', or 'Measurement' category as self (i.e. the same type
+                of underlying function).
+
+        Returns:
+            A ``SummedOp`` equivalent to the sum of self and other.
+        """
         if self == other:
             return self.mul(2.0)
-        elif isinstance(other, SummedOp):
-            self_new_ops = [op.mul(self.coeff) for op in self.oplist]
-            other_new_ops = [op.mul(other.coeff) for op in other.oplist]
-            return SummedOp(self_new_ops + other_new_ops)
-        elif other in self.oplist:
-            new_oplist = copy.copy(self.oplist)
-            other_index = self.oplist.index(other)
-            new_oplist[other_index] = new_oplist[other_index] + other
-            return SummedOp(new_oplist, coeff=self.coeff)
-        return SummedOp(self.oplist + [other], coeff=self.coeff)
+
+        self_new_ops = self.oplist if self.coeff == 1 \
+            else [op.mul(self.coeff) for op in self.oplist]
+        if isinstance(other, SummedOp):
+            other_new_ops = other.oplist if other.coeff == 1 \
+                else [op.mul(other.coeff) for op in other.oplist]
+        else:
+            other_new_ops = [other]
+        return SummedOp(self_new_ops + other_new_ops)
+
+    def simplify(self) -> 'SummedOp':
+        """Return Operator by simplifying duplicate operators.
+
+        E.g., ``SummedOp([2 * X ^ Y, X ^ Y]).simplify() -> SummedOp([3 * X ^ Y])``.
+
+        Returns:
+            A simplified ``SummedOp`` equivalent to self.
+        """
+        oplist = []
+        coeffs = []
+        for op in self.oplist:
+            if isinstance(op, PrimitiveOp):
+                new_op = PrimitiveOp(op.primitive)
+                new_coeff = op.coeff * self.coeff
+                if new_op in oplist:
+                    index = oplist.index(new_op)
+                    coeffs[index] += new_coeff
+                else:
+                    oplist.append(new_op)
+                    coeffs.append(new_coeff)
+            else:
+                if op in oplist:
+                    index = oplist.index(op)
+                    coeffs[index] += self.coeff
+                else:
+                    oplist.append(op)
+                    coeffs.append(self.coeff)
+        return SummedOp([op * coeff for op, coeff in zip(oplist, coeffs)])
 
     # Try collapsing list or trees of Sums.
     # TODO be smarter about the fact that any two ops in oplist could be evaluated for sum.
@@ -84,7 +127,7 @@ def to_legacy_op(self, massive: bool = False) -> LegacyBaseOperator:
         # We do this recursively in case there are SummedOps of PauliOps in oplist.
         legacy_ops = [op.to_legacy_op(massive=massive) for op in self.oplist]
 
-        if not all([isinstance(op, WeightedPauliOperator) for op in legacy_ops]):
+        if not all(isinstance(op, WeightedPauliOperator) for op in legacy_ops):
             # If any Operators in oplist cannot be represented by Legacy Operators, the error
             # will be raised in the offending matrix-converted result (e.g. StateFn or ListOp)
             return self.to_matrix_op(massive=massive).to_legacy_op(massive=massive)

qiskit/chemistry/drivers/fcidumpd/dumper.py

@@ -41,8 +41,8 @@ def dump(outpath: str, norb: int, nelec: int, hijs: List[float], hijkls: List[fl
     hij, hij_b = hijs
     hijkl, hijkl_ba, hijkl_bb = hijkls
     # assert that either all beta variables are None or all of them are not
-    assert all([h is None for h in [hij_b, hijkl_ba, hijkl_bb]]) \
-        or all([h is not None for h in [hij_b, hijkl_ba, hijkl_bb]])
+    assert all(h is None for h in [hij_b, hijkl_ba, hijkl_bb]) \
+        or all(h is not None for h in [hij_b, hijkl_ba, hijkl_bb])
     assert norb == hij.shape[0] == hijkl.shape[0]
     mos = range(norb)
     with open(outpath, 'w') as outfile:

qiskit/chemistry/drivers/fcidumpd/fcidumpdriver.py

@@ -54,7 +54,7 @@ def __init__(self, fcidump_input: str, atoms: Optional[List[str]] = None) -> Non
         self._fcidump_input = fcidump_input
 
         if atoms and not isinstance(atoms, list) \
-                and not all([sym in QMolecule.symbols for sym in atoms]):
+                and not all(sym in QMolecule.symbols for sym in atoms):
             raise QiskitChemistryError(
                 "The atoms must be a list of valid atomic symbols, not '{}'".format(atoms))
         self.atoms = atoms

qiskit/optimization/converters/quadratic_program_to_qubo.py

@@ -110,8 +110,8 @@ def get_compatibility_msg(problem: QuadraticProgram) -> str:
             msg += 'Continuous variables are not supported! '
 
         # check whether there are incompatible constraint types
-        if not all([constraint.sense == Constraint.Sense.EQ
-                    for constraint in problem.linear_constraints]):
+        if not all(constraint.sense == Constraint.Sense.EQ
+                   for constraint in problem.linear_constraints):
             msg += 'Only linear equality constraints are supported.'
         if len(problem.quadratic_constraints) > 0:
             msg += 'Quadratic constraints are not supported. '

test/aqua/operators/test_abelian_grouper.py

@@ -12,76 +12,68 @@
 # copyright notice, and modified files need to carry a notice indicating
 # that they have been altered from the originals.
 
-""" Test Abelian Grouper """
+"""Test Abelian Grouper"""
 
 import random
 import unittest
 from itertools import combinations
 from test.aqua import QiskitAquaTestCase
 
+from ddt import ddt, data, unpack
+
 from qiskit.aqua.operators import (X, Y, Z, I, AbelianGrouper)
 
 
+@ddt
 class TestAbelianGrouper(QiskitAquaTestCase):
     """Abelian Grouper tests."""
 
-    def test_abelian_grouper(self):
-        """ abelian grouper test """
-        paulis = (-1.052373245772859 * I ^ I) + \
-                 (0.39793742484318045 * I ^ Z) + \
-                 (-0.39793742484318045 * Z ^ I) + \
-                 (-0.01128010425623538 * Z ^ Z) + \
-                 (0.18093119978423156 * X ^ X)
-        grouped_sum = AbelianGrouper().convert(paulis)
-        self.assertEqual(len(grouped_sum.oplist), 2)
-        for group in grouped_sum:
-            for op_1, op_2 in combinations(group, 2):
-                self.assertTrue(op_1.commutes(op_2))
-
-    def test_abelian_grouper2(self):
-        """ abelian grouper test 2 """
-        paulis = (I ^ I ^ X ^ X * 0.2) + \
-                 (Z ^ Z ^ X ^ X * 0.3) + \
-                 (Z ^ Z ^ Z ^ Z * 0.4) + \
-                 (X ^ X ^ Z ^ Z * 0.5) + \
-                 (X ^ X ^ X ^ X * 0.6) + \
-                 (I ^ X ^ X ^ X * 0.7)
+    @data('h2_op', 'generic')
+    def test_abelian_grouper(self, pauli_op):
+        """Abelian grouper test"""
+        if pauli_op == 'h2_op':
+            paulis = (-1.052373245772859 * I ^ I) + \
+                     (0.39793742484318045 * I ^ Z) + \
+                     (-0.39793742484318045 * Z ^ I) + \
+                     (-0.01128010425623538 * Z ^ Z) + \
+                     (0.18093119978423156 * X ^ X)
+            num_groups = 2
+        else:
+            paulis = (I ^ I ^ X ^ X * 0.2) + \
+                     (Z ^ Z ^ X ^ X * 0.3) + \
+                     (Z ^ Z ^ Z ^ Z * 0.4) + \
+                     (X ^ X ^ Z ^ Z * 0.5) + \
+                     (X ^ X ^ X ^ X * 0.6) + \
+                     (I ^ X ^ X ^ X * 0.7)
+            num_groups = 4
         grouped_sum = AbelianGrouper().convert(paulis)
-        self.assertEqual(len(grouped_sum.oplist), 4)
+        self.assertEqual(len(grouped_sum.oplist), num_groups)
         for group in grouped_sum:
             for op_1, op_2 in combinations(group, 2):
                 self.assertTrue(op_1.commutes(op_2))
 
-    def test_abelian_grouper3(self):
-        """ abelian grouper test 3 """
+    @data((True, True), (True, False), (False, True), (False, False))
+    @unpack
+    def test_group_subops(self, fast, use_nx):
+        """grouper subroutine test"""
         paulis = (I ^ X) + (2 * X ^ X) + (3 * Z ^ Y)
-        for fast in [True, False]:
-            for use_nx in [True, False]:
-                grouped_sum = AbelianGrouper.group_subops(paulis, fast=fast, use_nx=use_nx)
-                self.assertEqual(len(grouped_sum), 2)
-                self.assertEqual(len(grouped_sum[0]), 2)
-                self.assertEqual(str(grouped_sum[0][0].primitive), 'IX')
-                self.assertEqual(grouped_sum[0][0].coeff, 1)
-                self.assertEqual(str(grouped_sum[0][1].primitive), 'XX')
-                self.assertEqual(grouped_sum[0][1].coeff, 2)
-                self.assertEqual(len(grouped_sum[1]), 1)
-                self.assertEqual(str(grouped_sum[1][0].primitive), 'ZY')
-                self.assertEqual(grouped_sum[1][0].coeff, 3)
+        grouped_sum = AbelianGrouper.group_subops(paulis, fast=fast, use_nx=use_nx)
+        with self.subTest('test group subops 1'):
+            self.assertEqual(len(grouped_sum), 2)
+            self.assertListEqual([str(op.primitive) for op in grouped_sum[0]], ['IX', 'XX'])
+            self.assertListEqual([op.coeff for op in grouped_sum[0]], [1, 2])
+            self.assertListEqual([str(op.primitive) for op in grouped_sum[1]], ['ZY'])
+            self.assertListEqual([op.coeff for op in grouped_sum[1]], [3])
 
-    def test_abelian_grouper4(self):
-        """ abelian grouper test 4 """
         paulis = X + (2 * Y) + (3 * Z)
-        grouped_sum = AbelianGrouper.group_subops(paulis)
-        self.assertEqual(len(grouped_sum), 3)
-        self.assertEqual(str(grouped_sum[0][0].primitive), 'X')
-        self.assertEqual(grouped_sum[0][0].coeff, 1)
-        self.assertEqual(str(grouped_sum[1][0].primitive), 'Y')
-        self.assertEqual(grouped_sum[1][0].coeff, 2)
-        self.assertEqual(str(grouped_sum[2][0].primitive), 'Z')
-        self.assertEqual(grouped_sum[2][0].coeff, 3)
+        grouped_sum = AbelianGrouper.group_subops(paulis, fast=fast, use_nx=use_nx)
+        with self.subTest('test group subops 2'):
+            self.assertEqual(len(grouped_sum), 3)
+            self.assertListEqual([str(op[0].primitive) for op in grouped_sum], ['X', 'Y', 'Z'])
+            self.assertListEqual([op[0].coeff for op in grouped_sum], [1, 2, 3])
 
     def test_abelian_grouper_random(self):
-        """ abelian grouper test with random paulis """
+        """Abelian grouper test with random paulis"""
         random.seed(1234)
         k = 10  # size of pauli operators
         n = 100  # number of pauli operators