Fix 4-pi periodicity of controlled rotations in the CommutationChecker

crates/accelerate/src/commutation_checker.rs

@@ -36,8 +36,6 @@ use qiskit_circuit::{BitType, Clbit, Qubit};
 use crate::unitary_compose;
 use crate::QiskitError;
 
-const TWOPI: f64 = 2.0 * std::f64::consts::PI;
-
 // These gates do not commute with other gates, we do not check them.
 static SKIPPED_NAMES: [&str; 4] = ["measure", "reset", "delay", "initialize"];
 
@@ -50,23 +48,38 @@ static SUPPORTED_OP: Lazy<HashSet<&str>> = Lazy::new(|| {
     ])
 });
 
-// Map rotation gates to their generators, or to ``None`` if we cannot currently efficiently
-// represent the generator in Rust and store the commutation relation in the commutation dictionary
-static SUPPORTED_ROTATIONS: Lazy<HashMap<&str, Option<OperationRef>>> = Lazy::new(|| {
+// Map rotation gates to their generators (or to ``None`` if we cannot currently efficiently
+// represent the generator in Rust and store the commutation relation in the commutation dictionary)
+// and their pi-periodicity. Here we mean a gate is n-pi periodic, if for angles that are
+// multiples of n*pi, the gate is equal to the identity up to a global phase.
+// E.g. RX is generated by X and 2-pi periodic, while CRX is generated by CX and 4-pi periodic.
+static SUPPORTED_ROTATIONS: Lazy<HashMap<&str, (u8, Option<OperationRef>)>> = Lazy::new(|| {
     HashMap::from([
-        ("rx", Some(OperationRef::Standard(StandardGate::XGate))),
-        ("ry", Some(OperationRef::Standard(StandardGate::YGate))),
-        ("rz", Some(OperationRef::Standard(StandardGate::ZGate))),
-        ("p", Some(OperationRef::Standard(StandardGate::ZGate))),
-        ("u1", Some(OperationRef::Standard(StandardGate::ZGate))),
-        ("crx", Some(OperationRef::Standard(StandardGate::CXGate))),
-        ("cry", Some(OperationRef::Standard(StandardGate::CYGate))),
-        ("crz", Some(OperationRef::Standard(StandardGate::CZGate))),
-        ("cp", Some(OperationRef::Standard(StandardGate::CZGate))),
-        ("rxx", None), // None means the gate is in the commutation dictionary
-        ("ryy", None),
-        ("rzx", None),
-        ("rzz", None),
+        ("rx", (2, Some(OperationRef::Standard(StandardGate::XGate)))),
+        ("ry", (2, Some(OperationRef::Standard(StandardGate::YGate)))),
+        ("rz", (2, Some(OperationRef::Standard(StandardGate::ZGate)))),
+        ("p", (2, Some(OperationRef::Standard(StandardGate::ZGate)))),
+        ("u1", (2, Some(OperationRef::Standard(StandardGate::ZGate)))),
+        ("rxx", (2, None)), // None means the gate is in the commutation dictionary
+        ("ryy", (2, None)),
+        ("rzx", (2, None)),
+        ("rzz", (2, None)),
+        (
+            "crx",
+            (4, Some(OperationRef::Standard(StandardGate::CXGate))),
+        ),
+        (
+            "cry",
+            (4, Some(OperationRef::Standard(StandardGate::CYGate))),
+        ),
+        (
+            "crz",
+            (4, Some(OperationRef::Standard(StandardGate::CZGate))),
+        ),
+        (
+            "cp",
+            (2, Some(OperationRef::Standard(StandardGate::CZGate))),
+        ),
     ])
 });
 
@@ -636,12 +649,14 @@ fn map_rotation<'a>(
     tol: f64,
 ) -> (&'a OperationRef<'a>, &'a [Param], bool) {
     let name = op.name();
-    if let Some(generator) = SUPPORTED_ROTATIONS.get(name) {
+
+    if let Some((pi_multiple, generator)) = SUPPORTED_ROTATIONS.get(name) {
         // If the rotation angle is below the tolerance, the gate is assumed to
         // commute with everything, and we simply return the operation with the flag that
         // it commutes trivially.
         if let Param::Float(angle) = params[0] {
-            if (angle % TWOPI).abs() < tol {
+            let periodicity = (*pi_multiple as f64) * ::std::f64::consts::PI;
+            if (angle % periodicity).abs() < tol {
                 return (op, params, true);
             };
         };

releasenotes/notes/fix-4pi-periodic-commutations-3b89d1813513f613.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    The :class:`.CommutationChecker` did not handle commutations of the :class:`.CRXGate`,
+    :class:`.CRYGate` and :class:`.CRZGate` correctly for angles
+    :math:`\pi(4k + 2)` for :math:`k \in \mathbb Z`.
+    In these cases, the controlled rotations were falsely assumed to commute with any gate.
+    Now these gates correctly commute with any gate if the rotation angle is a multiple of
+    :math:`4\pi`.

test/python/circuit/test_commutation_checker.py

@@ -56,6 +56,7 @@
     XGate,
     ZGate,
     HGate,
+    UnitaryGate,
 )
 from qiskit.dagcircuit import DAGOpNode
 
@@ -64,14 +65,14 @@
     RYGate,
     RZGate,
     PhaseGate,
-    CRXGate,
-    CRYGate,
-    CRZGate,
-    CPhaseGate,
     RXXGate,
     RYYGate,
     RZZGate,
     RZXGate,
+    CRXGate,
+    CRYGate,
+    CRZGate,
+    CPhaseGate,
 ]
 
 
@@ -433,26 +434,25 @@ def test_cutoff_angles(self, gate_cls):
                 self.assertFalse(scc.commute(generic_gate, [0, 1], [], gate, qargs, []))
 
     @idata(ROTATION_GATES)
-    def test_rotation_mod_2pi(self, gate_cls):
-        """Test the rotations modulo 2pi commute with any gate."""
+    def test_controlled_rotation_mod_4pi(self, gate_cls):
+        """Test the rotations modulo 2pi (4pi for controlled-rx/y/z) commute with any gate."""
         generic_gate = HGate()  # does not commute with any rotation gate
-        even = np.arange(-6, 7, 2)
+        multiples = np.arange(-6, 7)
 
-        with self.subTest(msg="even multiples"):
-            for multiple in even:
+        for multiple in multiples:
+            with self.subTest(multiple=multiple):
                 gate = gate_cls(multiple * np.pi)
-                self.assertTrue(
-                    scc.commute(generic_gate, [0], [], gate, list(range(gate.num_qubits)), [])
-                )
+                numeric = UnitaryGate(gate.to_matrix())
 
-        odd = np.arange(-5, 6, 2)
-        with self.subTest(msg="odd multiples"):
-            for multiple in odd:
-                gate = gate_cls(multiple * np.pi)
-                self.assertFalse(
-                    scc.commute(generic_gate, [0], [], gate, list(range(gate.num_qubits)), [])
+                # compute a numeric reference, that doesn't go through any special cases and
+                # uses a matrix-based commutation check
+                expected = scc.commute(
+                    generic_gate, [0], [], numeric, list(range(gate.num_qubits)), []
                 )
 
+                result = scc.commute(generic_gate, [0], [], gate, list(range(gate.num_qubits)), [])
+                self.assertEqual(expected, result)
+
     def test_custom_gate(self):
         """Test a custom gate."""
         my_cx = NewGateCX()