Skip to content

Fix string and standard gate mismatch in commutation checker #13991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 12, 2025
Merged

Fix string and standard gate mismatch in commutation checker #13991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da53e59
Fix string and standard gate mismatch in commutation checker
mtreinish Mar 11, 2025
822f8be
Rename is_commutation_skipped() is_parameterized()
mtreinish Mar 11, 2025
50ac8f9
Merge branch 'main' into fix-standard-gate-commutation
mtreinish Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/accelerate/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
202 changes: 114 additions & 88 deletions crates/accelerate/src/commutation_checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::object_registry::PyObjectAsKey;
use smallvec::SmallVec;
use std::fmt::Debug;
Expand All @@ -33,57 +32,69 @@ use qiskit_circuit::imports::QI_OPERATOR;
use qiskit_circuit::object_registry::ObjectRegistry;
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
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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())) {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now a logical change since it will eagerly return false on instructions, which is fine here since we don't check commutations between directives and these don't have defined matrices. Should we also check for OperationRef::Operation here, which also doesn't have a matrix and is not in the commutation library (it isn't right?)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't view it as a logical change, just a small efficiency change while I was removing string usage, because we were matching a bit too restrictively before here. We know that no Instruction either standard (like measure, reset, etc) or from python (like Intialize). Since as you say we can't have a commutation any instruction as they're explicitly non-unitary.

I left Operation because I thought they could have a matrix (like Clifford) but looking at the operation trait for PyOperation we have it hard coded to None

qiskit/crates/circuit/src/operations.rs

Lines 2726 to 2728 in 6f317f9

fn matrix(&self, _params: &[Param]) -> Option<Array2<Complex64>> {
None
}
so you're correct we can add it here. It does make me wonder what we should do about Clifford though

Copy link
Contributor

@Cryoris Cryoris Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yes I based this off the Rust implementation for when operations always return None... but I'm fine keeping the code logically correct as is, since we can add operations that have matrices 👍🏻

(Otherwise it would be very easy to introduce a bug if we forget to remove the Operation case here once we add e.g. Cliffords to Rust)

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_parameterized(params1) || is_parameterized(params2) {
return Some(false);
}

Expand Down Expand Up @@ -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_parameterized(params: &[Param]) -> bool {
params
.iter()
.any(|x| matches!(x, Param::ParameterExpression(_)))
}

/// Check if a given operation can be mapped onto a generator.
Expand All @@ -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(
Expand Down
31 changes: 31 additions & 0 deletions test/python/transpiler/test_commutative_cancellation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()