From 6a07078c364af4469cf61e4b0775ac917c6e4a70 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 14 Jan 2025 10:38:16 +0100 Subject: [PATCH 1/3] start --- crates/accelerate/src/commutation_checker.rs | 66 +++++++++++++------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/crates/accelerate/src/commutation_checker.rs b/crates/accelerate/src/commutation_checker.rs index 52d4900efa5b..1e3bb94fbdae 100644 --- a/crates/accelerate/src/commutation_checker.rs +++ b/crates/accelerate/src/commutation_checker.rs @@ -59,10 +59,6 @@ static SUPPORTED_ROTATIONS: Lazy>> = Lazy::ne ("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), @@ -70,6 +66,16 @@ static SUPPORTED_ROTATIONS: Lazy>> = Lazy::ne ]) }); +static SUPPORTED_CONTROLLED_ROTATIONS: Lazy>> = + Lazy::new(|| { + HashMap::from([ + ("crx", Some(OperationRef::Standard(StandardGate::CXGate))), + ("cry", Some(OperationRef::Standard(StandardGate::CYGate))), + ("crz", Some(OperationRef::Standard(StandardGate::CZGate))), + ("cp", Some(OperationRef::Standard(StandardGate::CZGate))), + ]) + }); + fn get_bits( py: Python, bits1: &Bound, @@ -117,6 +123,7 @@ impl CommutationChecker { // Initialize sets before they are used in the commutation checker Lazy::force(&SUPPORTED_OP); Lazy::force(&SUPPORTED_ROTATIONS); + Lazy::force(&SUPPORTED_CONTROLLED_ROTATIONS); CommutationChecker { library: CommutationLibrary::new(standard_gate_commutations), cache: HashMap::new(), @@ -636,27 +643,40 @@ 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 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 { - return (op, 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); + let (generator, periodicity) = if let Some(generator) = SUPPORTED_ROTATIONS.get(name) { + (generator, TWOPI) + } else if let Some(generator) = SUPPORTED_CONTROLLED_ROTATIONS.get(name) { + (generator, 2. * TWOPI) + } else { + return (op, params, false); + }; + + // if SUPPORTED_ROTATIONS.contains_key(name) { + // let g + // } + + // 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] { + if (angle % periodicity).abs() < tol { + return (op, params, true); }; - return (op, &[], false); - } - (op, params, 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 (gate, &[], false); + }; + return (op, &[], false); + // } + // (op, params, false) } fn get_relative_placement( From b6c630f18110562a7e74b146d2b556c843386af2 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Wed, 15 Jan 2025 17:48:05 +0100 Subject: [PATCH 2/3] fix 4pi periodicity of controlled pauli rots --- crates/accelerate/src/commutation_checker.rs | 105 ++++++++---------- ...eriodic-commutations-3b89d1813513f613.yaml | 9 ++ .../circuit/test_commutation_checker.py | 36 +++--- 3 files changed, 75 insertions(+), 75 deletions(-) create mode 100644 releasenotes/notes/fix-4pi-periodic-commutations-3b89d1813513f613.yaml diff --git a/crates/accelerate/src/commutation_checker.rs b/crates/accelerate/src/commutation_checker.rs index 1e3bb94fbdae..df720a807a01 100644 --- a/crates/accelerate/src/commutation_checker.rs +++ b/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,32 +48,40 @@ static SUPPORTED_OP: Lazy> = 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>> = 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. 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)>> = 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))), - ("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))), + ), ]) }); -static SUPPORTED_CONTROLLED_ROTATIONS: Lazy>> = - Lazy::new(|| { - HashMap::from([ - ("crx", Some(OperationRef::Standard(StandardGate::CXGate))), - ("cry", Some(OperationRef::Standard(StandardGate::CYGate))), - ("crz", Some(OperationRef::Standard(StandardGate::CZGate))), - ("cp", Some(OperationRef::Standard(StandardGate::CZGate))), - ]) - }); - fn get_bits( py: Python, bits1: &Bound, @@ -123,7 +129,6 @@ impl CommutationChecker { // Initialize sets before they are used in the commutation checker Lazy::force(&SUPPORTED_OP); Lazy::force(&SUPPORTED_ROTATIONS); - Lazy::force(&SUPPORTED_CONTROLLED_ROTATIONS); CommutationChecker { library: CommutationLibrary::new(standard_gate_commutations), cache: HashMap::new(), @@ -644,39 +649,25 @@ fn map_rotation<'a>( ) -> (&'a OperationRef<'a>, &'a [Param], bool) { let name = op.name(); - let (generator, periodicity) = if let Some(generator) = SUPPORTED_ROTATIONS.get(name) { - (generator, TWOPI) - } else if let Some(generator) = SUPPORTED_CONTROLLED_ROTATIONS.get(name) { - (generator, 2. * TWOPI) - } else { - return (op, params, false); - }; - - // if SUPPORTED_ROTATIONS.contains_key(name) { - // let g - // } - - // 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] { - if (angle % periodicity).abs() < tol { - return (op, params, true); + if let Some((pi_multiple, generator)) = SUPPORTED_ROTATIONS.get(name) { + if let Param::Float(angle) = params[0] { + let periodicity = (*pi_multiple as f64) * ::std::f64::consts::PI; + if (angle % periodicity).abs() < tol { + return (op, 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); - }; - return (op, &[], false); - // } - // (op, params, 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 (gate, &[], false); + }; + return (op, &[], false); + } + (op, params, false) } fn get_relative_placement( diff --git a/releasenotes/notes/fix-4pi-periodic-commutations-3b89d1813513f613.yaml b/releasenotes/notes/fix-4pi-periodic-commutations-3b89d1813513f613.yaml new file mode 100644 index 000000000000..289c43cb422f --- /dev/null +++ b/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`. diff --git a/test/python/circuit/test_commutation_checker.py b/test/python/circuit/test_commutation_checker.py index b4f6a30d904c..7492dbe14b91 100644 --- a/test/python/circuit/test_commutation_checker.py +++ b/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 (controlled 4pi) 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() From 59dbf242067c6ff570e878922b36db9b626fa5c7 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Thu, 16 Jan 2025 15:40:50 +0100 Subject: [PATCH 3/3] review comments by Elena & Sasha --- crates/accelerate/src/commutation_checker.rs | 8 ++++++-- test/python/circuit/test_commutation_checker.py | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/accelerate/src/commutation_checker.rs b/crates/accelerate/src/commutation_checker.rs index df720a807a01..cb51e9600319 100644 --- a/crates/accelerate/src/commutation_checker.rs +++ b/crates/accelerate/src/commutation_checker.rs @@ -50,8 +50,9 @@ static SUPPORTED_OP: Lazy> = 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. E.g. RX is generated by X and 2-pi periodic, while CRX is generated -// by CX and 4-pi periodic. +// 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)>> = Lazy::new(|| { HashMap::from([ ("rx", (2, Some(OperationRef::Standard(StandardGate::XGate)))), @@ -650,6 +651,9 @@ fn map_rotation<'a>( let name = op.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] { let periodicity = (*pi_multiple as f64) * ::std::f64::consts::PI; if (angle % periodicity).abs() < tol { diff --git a/test/python/circuit/test_commutation_checker.py b/test/python/circuit/test_commutation_checker.py index 7492dbe14b91..d0a7fc8eeb47 100644 --- a/test/python/circuit/test_commutation_checker.py +++ b/test/python/circuit/test_commutation_checker.py @@ -435,7 +435,7 @@ def test_cutoff_angles(self, gate_cls): @idata(ROTATION_GATES) def test_controlled_rotation_mod_4pi(self, gate_cls): - """Test the rotations modulo 2pi (controlled 4pi) commute with any gate.""" + """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 multiples = np.arange(-6, 7)