Fix global phase handling in CommutativeCancellation (backport Qiskit#14956) (Qiskit#14961)

* Fix global phase handling in `CommutativeCancellation` (Qiskit#14956)

* fix cc phase

* Fix case if `P/U1` is the gate we reduce to

* Fix 4-eps case and add tests

Co-authored-by: Alexander Ivrii <[email protected]>

* fix no var_z_gate case

* review comments + reno

* fix accumulation of T/S to P/U1

* Update releasenotes/notes/fix-commcanc-phase-f68fbb428363f081.yaml

Co-authored-by: Alexander Ivrii <[email protected]>

---------

Co-authored-by: Alexander Ivrii <[email protected]>
(cherry picked from commit 1064921)

# Conflicts:
#	crates/transpiler/src/passes/commutation_cancellation.rs

* fix merge conflicts

---------

Co-authored-by: Julien Gacon <[email protected]>

Author: mergify[bot]

crates/transpiler/src/passes/commutation_cancellation.rs

@@ -10,7 +10,7 @@
 // copyright notice, and modified files need to carry a notice indicating
 // that they have been altered from the originals.
 
-use std::f64::consts::PI;
+use std::f64::consts::{FRAC_PI_2, FRAC_PI_4, PI};
 
 use hashbrown::{HashMap, HashSet};
 use pyo3::exceptions::PyRuntimeError;
@@ -19,19 +19,16 @@ use pyo3::{pyfunction, wrap_pyfunction, Bound, PyResult, Python};
 use rustworkx_core::petgraph::stable_graph::NodeIndex;
 use smallvec::{smallvec, SmallVec};
 
-use crate::commutation_checker::CommutationChecker;
 use qiskit_circuit::dag_circuit::{DAGCircuit, NodeType, Wire};
 use qiskit_circuit::operations::{Operation, Param, StandardGate};
 use qiskit_circuit::Qubit;
+use qiskit_synthesis::QiskitError;
 
 use super::analyze_commutations;
-use qiskit_synthesis::{euler_one_qubit_decomposer, QiskitError};
+use crate::commutation_checker::CommutationChecker;
 
 const _CUTOFF_PRECISION: f64 = 1e-5;
 static ROTATION_GATES: [&str; 4] = ["p", "u1", "rz", "rx"];
-static HALF_TURNS: [&str; 2] = ["z", "x"];
-static QUARTER_TURNS: [&str; 1] = ["s"];
-static EIGHTH_TURNS: [&str; 1] = ["t"];
 
 static VAR_Z_MAP: [(&str, StandardGate); 3] = [
     ("rz", StandardGate::RZ),
@@ -93,14 +90,39 @@ pub fn cancel_commutations(
                 .map(|(_, gate)| gate)
         })
         .or_else(|| {
+            // Fallback to the first matching key from basis if there is no match in dag.op_names
             basis.iter().find_map(|g| {
                 VAR_Z_MAP
                     .iter()
                     .find(|(key, _)| *key == g.as_str())
                     .map(|(_, gate)| gate)
             })
         });
-    // Fallback to the first matching key from basis if there is no match in dag.op_names
+
+    // RZ and P/U1 have a phase difference of angle/2, which we need to account for
+    let z_phase_shift = match z_var_gate {
+        Some(z_var_gate) => {
+            if z_var_gate == &StandardGate::RZ {
+                |gate_name: &str, angle: f64| -> f64 {
+                    if ["u1", "p"].contains(&gate_name) {
+                        angle / 2.
+                    } else {
+                        0.
+                    }
+                }
+            } else {
+                |gate_name: &str, angle: f64| -> f64 {
+                    if gate_name == "rz" {
+                        -angle / 2.
+                    } else {
+                        0.
+                    }
+                }
+            }
+        }
+        // if there's no z_var_gate detected, Z rotations are not merged, so we have no phase shifts
+        None => |_name: &str, _angle: f64| -> f64 { 0. },
+    };
 
     // Gate sets to be cancelled
     /* Traverse each qubit to generate the cancel dictionaries
@@ -219,38 +241,29 @@ pub fn cancel_commutations(
                     };
                     let node_op_name = node_op.op.name();
 
-                    let node_angle = if ROTATION_GATES.contains(&node_op_name) {
-                        match node_op.params_view().first() {
-                            Some(Param::Float(f)) => Ok(*f),
+                    let (node_angle, phase_shift) = if ROTATION_GATES.contains(&node_op_name) {
+                        let node_angle = match node_op.params_view().first() {
+                            Some(Param::Float(f)) => *f,
                             _ => return Err(QiskitError::new_err(format!(
                                 "Rotational gate with parameter expression encountered in cancellation {:?}",
                                 node_op.op
                             )))
-                        }
-                    } else if HALF_TURNS.contains(&node_op_name) {
-                        Ok(PI)
-                    } else if QUARTER_TURNS.contains(&node_op_name) {
-                        Ok(PI / 2.0)
-                    } else if EIGHTH_TURNS.contains(&node_op_name) {
-                        Ok(PI / 4.0)
+                        };
+                        let phase_shift = z_phase_shift(node_op_name, node_angle);
+                        Ok((node_angle, phase_shift))
                     } else {
-                        Err(PyRuntimeError::new_err(format!(
-                            "Angle for operation {} is not defined",
-                            node_op_name
-                        )))
-                    };
-                    total_angle += node_angle?;
-
-                    let Param::Float(new_phase) = node_op
-                        .op
-                        .definition(node_op.params_view())
-                        .unwrap()
-                        .global_phase()
-                        .clone()
-                    else {
-                        unreachable!()
-                    };
-                    total_phase += new_phase
+                        match node_op_name {
+                            "t" => Ok((FRAC_PI_4, z_phase_shift("p", FRAC_PI_4))),
+                            "s" => Ok((FRAC_PI_2, z_phase_shift("p", FRAC_PI_2))),
+                            "z" => Ok((PI, z_phase_shift("p", PI))),
+                            "x" => Ok((PI, FRAC_PI_2)),
+                            _ => Err(PyRuntimeError::new_err(format!(
+                                "Angle for operation {node_op_name} is not defined"
+                            ))),
+                        }
+                    }?;
+                    total_angle += node_angle;
+                    total_phase += phase_shift;
                 }
 
                 let new_op = match cancel_key.gate {
@@ -259,24 +272,21 @@ pub fn cancel_commutations(
                     _ => unreachable!(),
                 };
 
-                let gate_angle = euler_one_qubit_decomposer::mod_2pi(total_angle, 0.);
+                let pi_multiple = total_angle / PI;
 
-                let new_op_phase: f64 = if gate_angle.abs() > _CUTOFF_PRECISION {
-                    dag.insert_1q_on_incoming_qubit((*new_op, &[total_angle]), cancel_set[0]);
-                    let Param::Float(new_phase) = new_op
-                        .definition(&[Param::Float(total_angle)])
-                        .unwrap()
-                        .global_phase()
-                        .clone()
-                    else {
-                        unreachable!();
-                    };
-                    new_phase
+                let mod4 = pi_multiple.rem_euclid(4.);
+                if mod4 < _CUTOFF_PRECISION || (4. - mod4) < _CUTOFF_PRECISION {
+                    // if the angle is close to a 4-pi multiple (from above or below), then the
+                    // operator is equal to the identity
+                } else if (mod4 - 2.).abs() < _CUTOFF_PRECISION {
+                    // a 2-pi multiple has a phase of pi: RX(2pi) = RZ(2pi) = -I = I exp(i pi)
+                    total_phase -= PI;
                 } else {
-                    0.0
-                };
+                    // any other is not the identity and we add the gate
+                    dag.insert_1q_on_incoming_qubit((*new_op, &[total_angle]), cancel_set[0]);
+                }
 
-                dag.add_global_phase(&Param::Float(total_phase - new_op_phase))?;
+                dag.add_global_phase(&Param::Float(total_phase))?;
 
                 for node in cancel_set {
                     dag.remove_op_node(*node);

releasenotes/notes/fix-commcanc-phase-f68fbb428363f081.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixed several issues in the :class:`.CommutativeCancellation` transpiler pass (and thereby in
+    :func:`.transpile`), where the global phase of the circuit was not updated correctly.
+    In particular, merging an X-gate and an RX-gate introduced a phase mismatch,
+    while removing a Pauli rotation gate with angle of the form
+    :math:`(2 + 4k)\pi`, :math:`k \in \mathbb Z` incorrectly produced a phase shift of :math:`-1`.

test/python/transpiler/test_commutative_cancellation.py

@@ -80,6 +80,61 @@ def test_all_gates(self):
         expected.global_phase = 0.5
         self.assertEqual(expected, new_circuit)
 
+    def test_2pi_multiples(self):
+        """Test 2pi multiples are handled with the correct phase they introduce."""
+        for eps in [0, 1e-10, -1e-10]:
+            for sign in [-1, 1]:
+                qc = QuantumCircuit(1)
+                qc.rz(sign * np.pi + eps, 0)
+                qc.rz(sign * np.pi, 0)
+
+                with self.subTest(msg="single 2pi", sign=sign, eps=eps):
+                    tqc = CommutativeCancellation()(qc)
+                    self.assertEqual(0, len(tqc.count_ops()))
+                    self.assertAlmostEqual(np.pi, tqc.global_phase)
+
+            for sign_x in [-1, 1]:
+                for sign_z in [-1, 1]:
+                    qc = QuantumCircuit(2)
+                    qc.rx(sign_x * np.pi + eps, 0)
+                    qc.rx(sign_x * np.pi, 0)
+                    qc.rz(sign_z * np.pi, 1)
+                    qc.rz(sign_z * np.pi, 1)
+
+                    with self.subTest(msg="two 2pi", sign_x=sign_x, sign_z=sign_z, eps=eps):
+                        tqc = CommutativeCancellation()(qc)
+                        self.assertEqual(0, len(tqc.count_ops()))
+                        self.assertAlmostEqual(0, tqc.global_phase)
+
+    def test_4pi_multiples(self):
+        """Test 4pi multiples are removed w/o changing the global phase."""
+        for eps in [0, 1e-10, -1e-10]:
+            for sign in [-1, 1]:
+                qc = QuantumCircuit(1)
+                qc.rz(sign * np.pi + eps, 0)
+                qc.rz(sign * 6 * np.pi, 0)
+                qc.rz(sign * np.pi, 0)
+
+                with self.subTest(sign=sign, eps=eps):
+                    tqc = CommutativeCancellation()(qc)
+                    self.assertEqual(0, len(tqc.count_ops()))
+                    self.assertAlmostEqual(0, tqc.global_phase)
+
+    def test_fixed_rotation_accumulation(self):
+        """Test accumulating gates with fixed angles (T, S) works correctly."""
+        cc = CommutativeCancellation()
+
+        # test for U1, P and RZ as target gate
+        for gate_cls in [RZGate, PhaseGate, U1Gate]:
+            qc = QuantumCircuit(1)
+            gate = gate_cls(0.2)
+            qc.append(gate, [0])
+            qc.t(0)
+            qc.s(0)
+
+            tqc = cc(qc)
+            self.assertTrue(np.allclose(Operator(qc).data, Operator(tqc).data))
+
     def test_commutative_circuit1(self):
         """A simple circuit where three CNOTs commute, the first and the last cancel.
 
@@ -149,9 +204,10 @@ def test_consecutive_cnots2(self):
             )
         )
         new_circuit = passmanager.run(circuit)
-        expected = QuantumCircuit(qr)
+        expected = QuantumCircuit(qr, global_phase=np.pi)  # RX(2pi) = -I = exp(i pi) I
 
         self.assertEqual(expected, new_circuit)
+        self.assertTrue(np.allclose(Operator(circuit).data, Operator(expected).data))
 
     def test_2_alternating_cnots(self):
         """A simple circuit where nothing should be cancelled.
@@ -669,9 +725,9 @@ def test_simple_if_else(self):
             (test.clbits[0], True), base_test1.copy(), base_test2.copy(), test.qubits, test.clbits
         )
 
-        expected = QuantumCircuit(3, 3)
+        expected = QuantumCircuit(3, 3, global_phase=np.pi / 2)
         expected.h(0)
-        expected.rx(np.pi + 0.2, 0)
+        expected.rx(np.pi + 0.2, 0)  # transforming X into RX(pi) introduces a pi/2 global phase
         expected.measure(0, 0)
         expected.x(0)