Fix string and standard gate mismatch in commutation checker

mtreinish · mtreinish · commit da53e59e3160 · 2025-03-11T09:27:40.000-04:00
This commit reworks the internals of the CommutationChecker to not rely on operation name except for where we do a lookup by name in the commutation library provided (which is the only key available to support custom gates). This fixes the case where a custom gate that overloads the standard gate name, previously the code would assume it to be a standard gate and internally panic when it wasn't. When working with standard gates (or standard instructions) we don't need to rely on string matching because we can rely on the rust data model to do the heavy lifting for us. This commit moves all the explicit handling of standard gates to use the StandardGate type directly and makes this logic more robust. This also removes are usage of the once_cell library in qiskit-accelerate because it was used to create a lazy static hashsets of strings which are no longer needed because static lookup tables replace this when we stopped using string comparisons. Fixes Qiskit#13988
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/accelerate/Cargo.toml b/crates/accelerate/Cargo.toml
@@ -28,7 +28,6 @@ itertools.workspace = true
 qiskit-circuit.workspace = true
 thiserror.workspace = true
 ndarray-einsum = "0.8.0"
-once_cell = "1.20.3"
 rustiq-core = "0.0.10"
 bytemuck.workspace = true
 nalgebra.workspace = true
diff --git a/crates/accelerate/src/commutation_checker.rs b/crates/accelerate/src/commutation_checker.rs
@@ -15,7 +15,6 @@ use ndarray::linalg::kron;
 use ndarray::Array2;
 use num_complex::Complex64;
 use num_complex::ComplexFloat;
-use once_cell::sync::Lazy;
 use qiskit_circuit::bit_data::VarAsKey;
 use smallvec::SmallVec;
 use std::fmt::Debug;
@@ -33,57 +32,69 @@ use qiskit_circuit::dag_node::DAGOpNode;
 use qiskit_circuit::imports::QI_OPERATOR;
 use qiskit_circuit::operations::OperationRef::{Gate as PyGateType, Operation as PyOperationType};
 use qiskit_circuit::operations::{
-    get_standard_gate_names, Operation, OperationRef, Param, StandardGate,
+    Operation, OperationRef, Param, StandardGate, STANDARD_GATE_SIZE,
 };
 use qiskit_circuit::{BitType, Clbit, Qubit};
 
 use crate::gate_metrics;
 use crate::unitary_compose;
 use crate::QiskitError;
 
-// These gates do not commute with other gates, we do not check them.
-static SKIPPED_NAMES: [&str; 4] = ["measure", "reset", "delay", "initialize"];
+const fn build_supported_ops() -> [bool; STANDARD_GATE_SIZE] {
+    let mut lut = [false; STANDARD_GATE_SIZE];
+    lut[StandardGate::RXXGate as usize] = true;
+    lut[StandardGate::RYYGate as usize] = true;
+    lut[StandardGate::RZZGate as usize] = true;
+    lut[StandardGate::RZXGate as usize] = true;
+    lut[StandardGate::HGate as usize] = true;
+    lut[StandardGate::XGate as usize] = true;
+    lut[StandardGate::YGate as usize] = true;
+    lut[StandardGate::ZGate as usize] = true;
+    lut[StandardGate::SXGate as usize] = true;
+    lut[StandardGate::SXdgGate as usize] = true;
+    lut[StandardGate::TGate as usize] = true;
+    lut[StandardGate::TdgGate as usize] = true;
+    lut[StandardGate::SGate as usize] = true;
+    lut[StandardGate::SdgGate as usize] = true;
+    lut[StandardGate::CXGate as usize] = true;
+    lut[StandardGate::CYGate as usize] = true;
+    lut[StandardGate::CZGate as usize] = true;
+    lut[StandardGate::SwapGate as usize] = true;
+    lut[StandardGate::ISwapGate as usize] = true;
+    lut[StandardGate::ECRGate as usize] = true;
+    lut[StandardGate::CCXGate as usize] = true;
+    lut[StandardGate::CSwapGate as usize] = true;
+    lut
+}
 
-// We keep a hash-set of operations eligible for commutation checking. This is because checking
-// eligibility is not for free.
-static SUPPORTED_OP: Lazy<HashSet<&str>> = Lazy::new(|| {
-    HashSet::from([
-        "rxx", "ryy", "rzz", "rzx", "h", "x", "y", "z", "sx", "sxdg", "t", "tdg", "s", "sdg", "cx",
-        "cy", "cz", "swap", "iswap", "ecr", "ccx", "cswap",
-    ])
-});
+static SUPPORTED_OP: [bool; STANDARD_GATE_SIZE] = build_supported_ops();
 
 // 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, Option<OperationRef>>> = Lazy::new(|| {
-    HashMap::from([
-        ("rx", Some(OperationRef::StandardGate(StandardGate::XGate))),
-        ("ry", Some(OperationRef::StandardGate(StandardGate::YGate))),
-        ("rz", Some(OperationRef::StandardGate(StandardGate::ZGate))),
-        ("p", Some(OperationRef::StandardGate(StandardGate::ZGate))),
-        ("u1", Some(OperationRef::StandardGate(StandardGate::ZGate))),
-        ("rxx", None), // None means the gate is in the commutation dictionary
-        ("ryy", None),
-        ("rzx", None),
-        ("rzz", None),
-        (
-            "crx",
-            Some(OperationRef::StandardGate(StandardGate::CXGate)),
-        ),
-        (
-            "cry",
-            Some(OperationRef::StandardGate(StandardGate::CYGate)),
-        ),
-        (
-            "crz",
-            Some(OperationRef::StandardGate(StandardGate::CZGate)),
-        ),
-        ("cp", Some(OperationRef::StandardGate(StandardGate::CZGate))),
-    ])
-});
+const fn build_supported_rotations() -> [Option<Option<StandardGate>>; STANDARD_GATE_SIZE] {
+    let mut lut = [None; STANDARD_GATE_SIZE];
+    lut[StandardGate::RXGate as usize] = Some(Some(StandardGate::XGate));
+    lut[StandardGate::RYGate as usize] = Some(Some(StandardGate::YGate));
+    lut[StandardGate::RZGate as usize] = Some(Some(StandardGate::ZGate));
+    lut[StandardGate::PhaseGate as usize] = Some(Some(StandardGate::ZGate));
+    lut[StandardGate::U1Gate as usize] = Some(Some(StandardGate::ZGate));
+    lut[StandardGate::CRXGate as usize] = Some(Some(StandardGate::CXGate));
+    lut[StandardGate::CRYGate as usize] = Some(Some(StandardGate::CYGate));
+    lut[StandardGate::CRZGate as usize] = Some(Some(StandardGate::CZGate));
+    lut[StandardGate::CPhaseGate as usize] = Some(Some(StandardGate::CZGate));
+    // RXXGate, RYYGate, RZXGate, and RZZGate are supported by the commutation dictionary
+    lut[StandardGate::RXXGate as usize] = Some(None);
+    lut[StandardGate::RYYGate as usize] = Some(None);
+    lut[StandardGate::RZXGate as usize] = Some(None);
+    lut[StandardGate::RZZGate as usize] = Some(None);
+    lut
+}
+
+static SUPPORTED_ROTATIONS: [Option<Option<StandardGate>>; STANDARD_GATE_SIZE] =
+    build_supported_rotations();
 
 fn get_bits<T>(bits1: &Bound<PyTuple>, bits2: &Bound<PyTuple>) -> PyResult<(Vec<T>, Vec<T>)>
 where
@@ -132,8 +143,6 @@ impl CommutationChecker {
         gates: Option<HashSet<String>>,
     ) -> Self {
         // Initialize sets before they are used in the commutation checker
-        Lazy::force(&SUPPORTED_OP);
-        Lazy::force(&SUPPORTED_ROTATIONS);
         CommutationChecker {
             library: CommutationLibrary::new(standard_gate_commutations),
             cache: HashMap::new(),
@@ -287,14 +296,24 @@ impl CommutationChecker {
 
         // if we have rotation gates, we attempt to map them to their generators, for example
         // RX -> X or CPhase -> CZ
-        let (op1, params1, trivial1) = map_rotation(op1, params1, tol);
+        let (op1_gate, params1, trivial1) = map_rotation(op1, params1, tol);
         if trivial1 {
             return Ok(true);
         }
-        let (op2, params2, trivial2) = map_rotation(op2, params2, tol);
+        let op1 = if let Some(gate) = op1_gate {
+            &OperationRef::StandardGate(gate)
+        } else {
+            op1
+        };
+        let (op2_gate, params2, trivial2) = map_rotation(op2, params2, tol);
         if trivial2 {
             return Ok(true);
         }
+        let op2 = if let Some(gate) = op2_gate {
+            &OperationRef::StandardGate(gate)
+        } else {
+            op2
+        };
 
         if let Some(gates) = &self.gates {
             if !gates.is_empty() && (!gates.contains(op1.name()) || !gates.contains(op2.name())) {
@@ -339,14 +358,15 @@ impl CommutationChecker {
         // the cache for
         //  * gates we know are in the cache (SUPPORTED_OPS), or
         //  * standard gates with float params (otherwise we cannot cache them)
-        let standard_gates = get_standard_gate_names();
-        let is_cachable = |name: &str, params: &[Param]| {
-            SUPPORTED_OP.contains(name)
-                || (standard_gates.contains(&name)
-                    && params.iter().all(|p| matches!(p, Param::Float(_))))
+        let is_cachable = |op: &OperationRef, params: &[Param]| {
+            if let Some(gate) = op.standard_gate() {
+                SUPPORTED_OP[gate as usize] || params.iter().all(|p| matches!(p, Param::Float(_)))
+            } else {
+                false
+            }
         };
-        let check_cache = is_cachable(first_op.name(), first_params)
-            && is_cachable(second_op.name(), second_params);
+        let check_cache =
+            is_cachable(first_op, first_params) && is_cachable(second_op, second_params);
 
         if !check_cache {
             return self.commute_matmul(
@@ -544,11 +564,25 @@ fn commutation_precheck(
         return Some(false);
     }
 
-    if SUPPORTED_OP.contains(op1.name()) && SUPPORTED_OP.contains(op2.name()) {
-        return None;
+    if let Some(gate_1) = op1.standard_gate() {
+        if let Some(gate_2) = op2.standard_gate() {
+            if SUPPORTED_OP[gate_1 as usize] && SUPPORTED_OP[gate_2 as usize] {
+                return None;
+            }
+        }
+    }
+
+    if matches!(
+        op1,
+        OperationRef::StandardInstruction(_) | OperationRef::Instruction(_)
+    ) || matches!(
+        op2,
+        OperationRef::StandardInstruction(_) | OperationRef::Instruction(_)
+    ) {
+        return Some(false);
     }
 
-    if is_commutation_skipped(op1, params1) || is_commutation_skipped(op2, params2) {
+    if is_commutation_skipped(params1) || is_commutation_skipped(params2) {
         return Some(false);
     }
 
@@ -580,15 +614,10 @@ fn matrix_via_operator(py: Python, py_obj: &PyObject) -> PyResult<Array2<Complex
         .to_owned())
 }
 
-fn is_commutation_skipped<T>(op: &T, params: &[Param]) -> bool
-where
-    T: Operation,
-{
-    op.directive()
-        || SKIPPED_NAMES.contains(&op.name())
-        || params
-            .iter()
-            .any(|x| matches!(x, Param::ParameterExpression(_)))
+fn is_commutation_skipped(params: &[Param]) -> bool {
+    params
+        .iter()
+        .any(|x| matches!(x, Param::ParameterExpression(_)))
 }
 
 /// Check if a given operation can be mapped onto a generator.
@@ -604,36 +633,33 @@ fn map_rotation<'a>(
     op: &'a OperationRef<'a>,
     params: &'a [Param],
     tol: f64,
-) -> (&'a OperationRef<'a>, &'a [Param], bool) {
-    let name = op.name();
-
-    if let Some(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] {
-            let gate = op
-                .standard_gate()
-                .expect("Supported gates are standard gates");
-            let (tr_over_dim, dim) = gate_metrics::rotation_trace_and_dim(gate, angle)
-                .expect("All rotation should be covered at this point");
-            let gate_fidelity = tr_over_dim.abs().powi(2);
-            let process_fidelity = (dim * gate_fidelity + 1.) / (dim + 1.);
-            if (1. - process_fidelity).abs() <= tol {
-                return (op, params, true);
+) -> (Option<StandardGate>, &'a [Param], bool) {
+    if let Some(gate) = op.standard_gate() {
+        if let Some(generator) = SUPPORTED_ROTATIONS[gate as usize] {
+            // 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] {
+                let (tr_over_dim, dim) = gate_metrics::rotation_trace_and_dim(gate, angle)
+                    .expect("All rotation should be covered at this point");
+                let gate_fidelity = tr_over_dim.abs().powi(2);
+                let process_fidelity = (dim * gate_fidelity + 1.) / (dim + 1.);
+                if (1. - process_fidelity).abs() <= tol {
+                    return (Some(gate), params, true);
+                };
             };
-        };
 
-        // Otherwise we need to cover two cases -- either a generator is given, in which case
-        // we return it, or we don't have a generator yet, but we know we have the operation
-        // stored in the commutation library. For example, RXX does not have a generator in Rust
-        // yet (PauliGate is not in Rust currently), but it is stored in the library, so we
-        // can strip the parameters and just return the gate.
-        if let Some(gate) = generator {
-            return (gate, &[], false);
-        };
+            // Otherwise we need to cover two cases -- either a generator is given, in which case
+            // we return it, or we don't have a generator yet, but we know we have the operation
+            // stored in the commutation library. For example, RXX does not have a generator in Rust
+            // yet (PauliGate is not in Rust currently), but it is stored in the library, so we
+            // can strip the parameters and just return the gate.
+            if let Some(gate) = generator {
+                return (Some(gate), &[], false);
+            };
+        }
     }
-    (op, params, false)
+    (None, params, false)
 }
 
 fn get_relative_placement(
diff --git a/test/python/transpiler/test_commutative_cancellation.py b/test/python/transpiler/test_commutative_cancellation.py
@@ -772,6 +772,37 @@ def test_no_intransitive_cancellation(self):
         new_circuit = passmanager.run(circ)
         self.assertEqual(new_circuit, circ)
 
+    def test_overloaded_standard_gate_name(self):
+        """Validate the pass works with custom gates using overloaded names
+
+        See: https://github.com/Qiskit/qiskit/issues/13988 for more details.
+        """
+        qasm_str = """OPENQASM 2.0;
+include "qelib1.inc";
+gate ryy(param0) q0,q1
+{
+ rx(pi/2) q0;
+ rx(pi/2) q1;
+ cx q0,q1;
+ rz(0.37801308) q1;
+ cx q0,q1;
+ rx(-pi/2) q0;
+ rx(-pi/2) q1;
+}
+qreg q0[2];
+creg c0[2];
+z q0[0];
+ryy(1.2182379) q0[0],q0[1];
+z q0[0];
+measure q0[0] -> c0[0];
+measure q0[1] -> c0[1];
+"""
+        qc = QuantumCircuit.from_qasm_str(qasm_str)
+        cancellation_pass = CommutativeCancellation()
+        res = cancellation_pass(qc)
+        # We don't cancel any gates with a custom rzz gate
+        self.assertEqual(res.count_ops()["z"], 2)
+
 
 if __name__ == "__main__":
     unittest.main()
