Fix Optimize1qGatesDecomposition length heuristic (#6553)

* fix 1q optimization heuristic

* drop unnecessary brackets

Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>

* actually fill out reno template :P

* maker linter happier

* make lev happier

* add a GH link to the sloppy synth warning

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* improve source linking in changelog

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* remember target basis name

* update circuit definitions

* improve linter cheerfulness

* increase reluctance to decompose calibrated gates

* change .basis slot to ._decomposers

* my local linter thinks everything is fine :/

* add some U3 special cases

* make black happy

* fix a claimed circular import

* add a couple more rewrite tests

* satisfy black

* avoid lambda = ± pi in 1Q KAK

* optimize on strict length

* more thorough gate elision during 1Q synthesis

* respond to most of Lev's feedback

* add an 'allow_non_canonical' parameters

* normalize K rolls

* Update qiskit/quantum_info/synthesis/one_qubit_decompose.py

Co-authored-by: Lev Bishop <18673315+levbishop@users.noreply.github.com>

* Update qiskit/quantum_info/synthesis/one_qubit_decompose.py

Co-authored-by: Lev Bishop <18673315+levbishop@users.noreply.github.com>

* add some Euler special case tests for pushing a K(pi) through an A(alpha)

* add some Euler special case tests for pushing a K(pi) through an A(alpha)

* Update one_qubit_decompose.py

a by-hand attempt at reformatting the docstring

* ok linter

Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Lev Bishop <18673315+levbishop@users.noreply.github.com>

qiskit/quantum_info/synthesis/one_qubit_decompose.py

@@ -281,92 +281,121 @@ def _params_u1x(mat):
         return theta, phi, lam, phase - 0.5 * (theta + phi + lam)
 
     @staticmethod
-    def _circuit_zyz(theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL):
-        gphase = phase - (phi + lam) / 2
-        qr = QuantumRegister(1, "qr")
-        circuit = QuantumCircuit(qr)
-        if not simplify:
-            atol = -1.0
-        if abs(theta) < atol:
-            tot = _mod_2pi(phi + lam, atol)
-            if abs(tot) > atol:
-                circuit._append(RZGate(tot), [qr[0]], [])
-                gphase += tot / 2
-            circuit.global_phase = gphase
-            return circuit
-        if abs(theta - np.pi) < atol:
-            gphase += phi
-            lam, phi = lam - phi, 0
-        lam = _mod_2pi(lam, atol)
-        if abs(lam) > atol:
-            gphase += lam / 2
-            circuit._append(RZGate(lam), [qr[0]], [])
-        circuit._append(RYGate(theta), [qr[0]], [])
-        phi = _mod_2pi(phi, atol)
-        if abs(phi) > atol:
-            gphase += phi / 2
-            circuit._append(RZGate(phi), [qr[0]], [])
-        circuit.global_phase = gphase
-        return circuit
+    def _circuit_kak(
+        theta,
+        phi,
+        lam,
+        phase,
+        simplify=True,
+        atol=DEFAULT_ATOL,
+        allow_non_canonical=True,
+        k_gate=RZGate,
+        a_gate=RYGate,
+    ):
+        """
+        Installs the angles phi, theta, and lam into a KAK-type decomposition of the form
+        K(phi) . A(theta) . K(lam) , where K and A are an orthogonal pair drawn from RZGate, RYGate,
+        and RXGate.
 
-    @staticmethod
-    def _circuit_zxz(theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL):
-        gphase = phase - (phi + lam) / 2
-        qr = QuantumRegister(1, "qr")
-        circuit = QuantumCircuit(qr)
-        if not simplify:
-            atol = -1.0
-        if abs(theta) < atol:
-            tot = _mod_2pi(phi + lam)
-            if abs(tot) > atol:
-                circuit._append(RZGate(tot), [qr[0]], [])
-                gphase += tot / 2
-            circuit.global_phase = gphase
-            return circuit
-        if abs(theta - np.pi) < atol:
-            gphase += phi
-            lam, phi = lam - phi, 0
-        lam = _mod_2pi(lam, atol)
-        if abs(lam) > atol:
-            gphase += lam / 2
-            circuit._append(RZGate(lam), [qr[0]], [])
-        circuit._append(RXGate(theta), [qr[0]], [])
-        phi = _mod_2pi(phi, atol)
-        if abs(phi) > atol:
-            gphase += phi / 2
-            circuit._append(RZGate(phi), [qr[0]], [])
-        circuit.global_phase = gphase
-        return circuit
+        Args:
+            theta (float): The middle KAK parameter.  Expected to lie in [0, pi).
+            phi (float): The first KAK parameter.
+            lam (float): The final KAK parameter.
+            phase (float): The input global phase.
+            k_gate (Callable): The constructor for the K gate Instruction.
+            a_gate (Callable): The constructor for the A gate Instruction.
+            simplify (bool): Indicates whether gates should be elided / coalesced where possible.
+            allow_non_canonical (bool): Indicates whether we are permitted to reverse the sign of
+                the middle parameter, theta, in the output.  When this and `simplify` are both
+                enabled, we take the opportunity to commute half-rotations in the outer gates past
+                the middle gate, which permits us to coalesce them at the cost of reversing the sign
+                of theta.
 
-    @staticmethod
-    def _circuit_xyx(theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL):
+        Returns:
+            QuantumCircuit: The assembled circuit.
+        """
         gphase = phase - (phi + lam) / 2
         qr = QuantumRegister(1, "qr")
         circuit = QuantumCircuit(qr)
         if not simplify:
             atol = -1.0
+        # Early return for the middle-gate-free case
         if abs(theta) < atol:
-            tot = _mod_2pi(phi + lam, atol)
-            if abs(tot) > atol:
-                circuit._append(RXGate(tot), [qr[0]], [])
-                gphase += tot / 2
+            lam, phi = lam + phi, 0
+            # NOTE: The following normalization is safe, because the gphase correction below
+            #       fixes a particular diagonal entry to 1, which prevents any potential phase
+            #       slippage coming from _mod_2pi injecting multiples of 2pi.
+            lam = _mod_2pi(lam, atol)
+            if abs(lam) > atol:
+
+                circuit._append(k_gate(lam), [qr[0]], [])
+                gphase += lam / 2
             circuit.global_phase = gphase
             return circuit
         if abs(theta - np.pi) < atol:
             gphase += phi
             lam, phi = lam - phi, 0
+        if allow_non_canonical and (
+            abs(_mod_2pi(lam + np.pi)) < atol or abs(_mod_2pi(phi + np.pi)) < atol
+        ):
+            lam, theta, phi = lam + np.pi, -theta, phi + np.pi
         lam = _mod_2pi(lam, atol)
         if abs(lam) > atol:
             gphase += lam / 2
-            circuit._append(RXGate(lam), [qr[0]], [])
-        circuit._append(RYGate(theta), [qr[0]], [])
+            circuit._append(k_gate(lam), [qr[0]], [])
+        circuit._append(a_gate(theta), [qr[0]], [])
         phi = _mod_2pi(phi, atol)
         if abs(phi) > atol:
             gphase += phi / 2
-            circuit._append(RXGate(phi), [qr[0]], [])
+            circuit._append(k_gate(phi), [qr[0]], [])
         circuit.global_phase = gphase
         return circuit
 
+    def _circuit_zyz(
+        self, theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL, allow_non_canonical=True
+    ):
+        return self._circuit_kak(
+            theta,
+            phi,
+            lam,
+            phase,
+            simplify=simplify,
+            atol=atol,
+            allow_non_canonical=allow_non_canonical,
+            k_gate=RZGate,
+            a_gate=RYGate,
+        )
+
+    def _circuit_zxz(
+        self, theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL, allow_non_canonical=True
+    ):
+        return self._circuit_kak(
+            theta,
+            phi,
+            lam,
+            phase,
+            simplify=simplify,
+            atol=atol,
+            allow_non_canonical=allow_non_canonical,
+            k_gate=RZGate,
+            a_gate=RXGate,
+        )
+
+    def _circuit_xyx(
+        self, theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL, allow_non_canonical=True
+    ):
+        return self._circuit_kak(
+            theta,
+            phi,
+            lam,
+            phase,
+            simplify=simplify,
+            atol=atol,
+            allow_non_canonical=allow_non_canonical,
+            k_gate=RXGate,
+            a_gate=RYGate,
+        )
+
     @staticmethod
     def _circuit_u3(theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL):
         qr = QuantumRegister(1, "qr")
@@ -407,35 +436,44 @@ def _circuit_u(theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL):
 
     @staticmethod
     def _circuit_psx_gen(theta, phi, lam, phase, atol, pfun, xfun, xpifun=None):
-        """Generic X90, phase decomposition"""
+        """
+        Generic X90, phase decomposition
+
+        NOTE: `pfun` is responsible for eliding gates where appropriate (e.g., at angle value 0).
+        """
         qr = QuantumRegister(1, "qr")
         circuit = QuantumCircuit(qr, global_phase=phase)
-        # Check for decomposition into minimimal number required SX pulses
+        # Early return for zero SX decomposition
         if np.abs(theta) < atol:
-            # Zero SX gate decomposition
             pfun(circuit, qr, lam + phi)
             return circuit
+        # Early return for single SX decomposition
         if abs(theta - np.pi / 2) < atol:
-            # Single SX gate decomposition
             pfun(circuit, qr, lam - np.pi / 2)
             xfun(circuit, qr)
             pfun(circuit, qr, phi + np.pi / 2)
             return circuit
-        # General two-SX gate decomposition
-        # Shift theta and phi so decomposition is
-        # P(phi).SX.P(theta).SX.P(lam)
+        # General double SX decomposition
         if abs(theta - np.pi) < atol:
             circuit.global_phase += lam
             phi, lam = phi - lam, 0
+        if abs(_mod_2pi(lam + np.pi)) < atol or abs(_mod_2pi(phi)) < atol:
+            lam, theta, phi = lam + np.pi, -theta, phi + np.pi
+            circuit.global_phase -= theta
+        # Shift theta and phi to turn the decomposition from
+        # RZ(phi).RY(theta).RZ(lam) = RZ(phi).RX(-pi/2).RZ(theta).RX(pi/2).RZ(lam)
+        # into RZ(phi+pi).SX.RZ(theta+pi).SX.RZ(lam) .
+        theta, phi = theta + np.pi, phi + np.pi
         circuit.global_phase -= np.pi / 2
+        # Emit circuit
         pfun(circuit, qr, lam)
-        if xpifun and abs(_mod_2pi(theta + np.pi)) < atol:
+        if xpifun and abs(_mod_2pi(theta)) < atol:
             xpifun(circuit, qr)
         else:
             xfun(circuit, qr)
-            pfun(circuit, qr, theta + np.pi)
+            pfun(circuit, qr, theta)
             xfun(circuit, qr)
-        pfun(circuit, qr, phi + np.pi)
+        pfun(circuit, qr, phi)
 
         return circuit
 

qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py

@@ -14,13 +14,13 @@
 
 import copy
 import logging
-import math
+import warnings
 
 import numpy as np
 
+from qiskit.circuit.library.standard_gates import U3Gate
 from qiskit.transpiler.basepasses import TransformationPass
 from qiskit.quantum_info.synthesis import one_qubit_decompose
-from qiskit.circuit.library.standard_gates import U3Gate
 from qiskit.converters import circuit_to_dag
 
 logger = logging.getLogger(__name__)
@@ -38,25 +38,26 @@ def __init__(self, basis=None):
                 and the Euler basis.
         """
         super().__init__()
-        self.basis = None
+        self._target_basis = basis
+        self._decomposers = None
         if basis:
-            self.basis = []
+            self._decomposers = []
             basis_set = set(basis)
             euler_basis_gates = one_qubit_decompose.ONE_QUBIT_EULER_BASIS_GATES
             for euler_basis_name, gates in euler_basis_gates.items():
                 if set(gates).issubset(basis_set):
-                    basis_copy = copy.copy(self.basis)
+                    basis_copy = copy.copy(self._decomposers)
                     for base in basis_copy:
                         # check if gates are a superset of another basis
                         # and if so, remove that basis
                         if set(euler_basis_gates[base.basis]).issubset(set(gates)):
-                            self.basis.remove(base)
+                            self._decomposers.remove(base)
                         # check if the gates are a subset of another basis
                         elif set(gates).issubset(set(euler_basis_gates[base.basis])):
                             break
                     # if not a subset, add it to the list
                     else:
-                        self.basis.append(
+                        self._decomposers.append(
                             one_qubit_decompose.OneQubitEulerDecomposer(euler_basis_name)
                         )
 
@@ -69,40 +70,76 @@ def run(self, dag):
         Returns:
             DAGCircuit: the optimized DAG.
         """
-        if not self.basis:
+        if not self._decomposers:
             logger.info("Skipping pass because no basis is set")
             return dag
         runs = dag.collect_1q_runs()
-        identity_matrix = np.eye(2)
         for run in runs:
-            single_u3 = False
-            # Don't try to optimize a single 1q gate, except for U3
-            if len(run) <= 1:
-                params = run[0].op.params
-                # Remove single identity gates
-                if len(params) > 0 and np.array_equal(run[0].op.to_matrix(), identity_matrix):
+            # SPECIAL CASE: Don't bother to optimize single U3 gates which are in the basis set.
+            #     The U3 decomposer is only going to emit a sequence of length 1 anyhow.
+            if "u3" in self._target_basis and len(run) == 1 and isinstance(run[0].op, U3Gate):
+                # Toss U3 gates equivalent to the identity; there we get off easy.
+                if np.array_equal(run[0].op.to_matrix(), np.eye(2)):
                     dag.remove_op_node(run[0])
                     continue
-                if isinstance(run[0].op, U3Gate):
-                    param = float(params[0])
-                    if math.isclose(param, 0, rel_tol=0, abs_tol=1e-12) or math.isclose(
-                        param, np.pi / 2, abs_tol=1e-12, rel_tol=0
-                    ):
-                        single_u3 = True
-                    else:
-                        continue
-                else:
+                # We might rewrite into lower `u`s if they're available.
+                if "u2" not in self._target_basis and "u1" not in self._target_basis:
                     continue
 
             new_circs = []
             operator = run[0].op.to_matrix()
             for gate in run[1:]:
                 operator = gate.op.to_matrix().dot(operator)
-            for decomposer in self.basis:
+            for decomposer in self._decomposers:
                 new_circs.append(decomposer._decompose(operator))
             if new_circs:
                 new_circ = min(new_circs, key=len)
-                if len(run) > len(new_circ) or (single_u3 and new_circ.data[0][0].name != "u3"):
+
+                # do we even have calibrations?
+                has_cals_p = dag.calibrations is not None and len(dag.calibrations) > 0
+                # is this run all in the target set and also uncalibrated?
+                rewriteable_and_in_basis_p = all(
+                    g.name in self._target_basis
+                    and (not has_cals_p or not dag.has_calibration_for(g))
+                    for g in run
+                )
+                # does this run have uncalibrated gates?
+                uncalibrated_p = not has_cals_p or any(not dag.has_calibration_for(g) for g in run)
+                # does this run have gates not in the image of ._decomposers _and_ uncalibrated?
+                uncalibrated_and_not_basis_p = any(
+                    g.name not in self._target_basis
+                    and (not has_cals_p or not dag.has_calibration_for(g))
+                    for g in run
+                )
+
+                if rewriteable_and_in_basis_p and len(run) < len(new_circ):
+                    # NOTE: This is short-circuited on calibrated gates, which we're timid about
+                    #       reducing.
+                    warnings.warn(
+                        f"Resynthesized {run} and got {new_circ}, "
+                        f"but the original was native and the new value is longer.  This "
+                        f"indicates an efficiency bug in synthesis.  Please report it by "
+                        f"opening an issue here: "
+                        f"https://github.com/Qiskit/qiskit-terra/issues/new/choose",
+                        stacklevel=2,
+                    )
+                # if we're outside of the basis set, we're obligated to logically decompose.
+                # if we're outside of the set of gates for which we have physical definitions,
+                #    then we _try_ to decompose, using the results if we see improvement.
+                # NOTE: Here we use circuit length as a weak proxy for "improvement"; in reality,
+                #       we care about something more like fidelity at runtime, which would mean,
+                #       e.g., a preference for `RZGate`s over `RXGate`s.  In fact, users sometimes
+                #       express a preference for a "canonical form" of a circuit, which may come in
+                #       the form of some parameter values, also not visible at the level of circuit
+                #       length.  Since we don't have a framework for the caller to programmatically
+                #       express what they want here, we include some special casing for particular
+                #       gates which we've promised to normalize --- but this is fragile and should
+                #       ultimately be done away with.
+                if (
+                    uncalibrated_and_not_basis_p
+                    or (uncalibrated_p and len(run) > len(new_circ))
+                    or isinstance(run[0].op, U3Gate)
+                ):
                     new_dag = circuit_to_dag(new_circ)
                     dag.substitute_node_with_dag(run[0], new_dag)
                     # Delete the other nodes in the run

releasenotes/notes/fixed-bug-in-Optimize1qGatesDecomposition-skipping-short-sequences-044a64740bf414a7.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes a bug in :func:`~qiskit.transpiler.passes.Optimize1qGatesDecomposition` previously causing certain
+    short sequences of gates to erroneously not be rewritten.