Support commutation check between Pauli-based gates and standard gates#15488
Support commutation check between Pauli-based gates and standard gates#15488ShellyGarion merged 42 commits intoQiskit:mainfrom
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 22054543433Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
d5fd9eb to
063210b
Compare
| use BitTerm::*; | ||
| [ | ||
| // Id | ||
| &[], | ||
|
|
||
| // Rx | ||
| &[X], | ||
|
|
||
| // Ry | ||
| &[Y], | ||
|
|
||
| // Rz | ||
| &[Z], | ||
|
|
||
| // H (roughly X+Z in this picture) | ||
| &[X, Z], | ||
|
|
||
| // S | ||
| &[Z], | ||
|
|
||
| // Sdg | ||
| &[Z], | ||
| ] |
There was a problem hiding this comment.
Hi Debasmita! Since you already used an enum for the standard gates, do you think it would be a good idea to maybe restructure this a bit and use a match statement instead to enforce that all gates have a valid representation, else it might just throw an indexing error in the future?
There was a problem hiding this comment.
The introduced StandardGate enum above should just be a placeholder, instead we should use the existing StandardGate enum in operation.rs that defines all standard gates. It's fine to use a lookup table if it contains all standard gates, since we know the indices are valid 🙂
| }; | ||
|
|
||
| // For now assume a single-qubit generator acting on 1 qubit. | ||
| let num_qubits = 1; |
There was a problem hiding this comment.
Once we're satisfied with the pipeline, meaning that (1) the light cone pass works for large numbers of qubits and (2) the ASV benchmarks show we are not slower, then we should add all other standard gates here.
| def test_light_cone_index_out_of_bounds(self): | ||
| """Test scanning a circuit with dispersed indices does not panic. | ||
| See https://github.com/Qiskit/qiskit/issues/15021 | ||
| """ | ||
| qc = QuantumCircuit(18) | ||
| qc.rx(np.pi / 3, range(0, qc.num_qubits)) | ||
| bit_terms = "ZZZZZZZZZ" | ||
| indices = [0, 1, 2, 3, 4, 9, 10, 12, 13] | ||
| pm = PassManager( | ||
| [ | ||
| LightCone( | ||
| bit_terms=bit_terms, | ||
| indices=indices, | ||
| ) | ||
| ] | ||
| ) | ||
| # This should not raise a PanicException or IndexError | ||
| reduced_circ = pm.run(qc) | ||
| self.assertIsInstance(reduced_circ, QuantumCircuit) |
There was a problem hiding this comment.
Yeah that's the right direction 👍🏻 Two small comments on that test below
| /// | ||
| /// `None` means “no special handling, use the generic commutation path”. | ||
| pub fn generator_observable(gate: StandardGate) -> Option<SparseObservable> { | ||
| let terms: &[BitTerm] = match gate { |
There was a problem hiding this comment.
Reminder that we want to change this into a static lookup table for all StandardGates
There was a problem hiding this comment.
Yes, I will modify this file to use a static array (like GENERATOR_LUT) for mapping standard gates to bit terms, instead of the match statement.
|
Ran the ASV benchmarks: Benchmarks show no regression. Performance is either identical (ratio 1.00) or slightly improved (ratio ~0.94)
Note on Timeouts: |
ShellyGarion
left a comment
There was a problem hiding this comment.
Hi @debasmita2102 - thanks for this PR!
I have a few suggestions.
There was a problem hiding this comment.
I checked all 2-qubit gates and added a few comments and suggestions.
In order to check the commutativity, I cheked commutativity with all 2-qubit paulis.
Perhaps it's worth to add this as a test? (for all 1-qubit and 2-qubit gates, and partially for 3-4-5 qubit gates)?
for pauli in ['XX', 'XY', 'XZ', 'YY', 'YX', 'YZ', 'ZZ', 'ZX', 'ZY', 'IX', 'IY', 'IZ', 'XI', 'YI', 'ZI']: # all 15 non-trivial paulis
evo = PauliEvolutionGate(SparsePauliOp(pauli), time=1.0)
print (pauli, scc.commute(CXGate(), [0, 1], [], evo, [0, 1], []),
Pauli(pauli).commutes(Pauli('XI')) & Pauli(pauli).commutes(Pauli('IZ')) & Pauli(pauli).commutes(Pauli('XZ')))
P.S. I think that the test suggested by Alexander Ivrii below is actually better and more accurate, as it compares the exact operators. Commutativity tests may fail if we take sums of Paulis for example.
So, you don't need to implememnt these tests.
| let (definition, num_qubits): (&[&[(u32, BitTerm)]], u32) = match gate { | ||
| // Single Qubit Gates (act on q0) | ||
| // X-type: [X] | ||
| StandardGate::X | StandardGate::RX | StandardGate::SX | StandardGate::SXdg | StandardGate::R => (&[&[(0, BitTerm::X)]], 1), |
There was a problem hiding this comment.
RGate has 2 parameters, so I don't think this check is enough.
Like other gates with more than 1-parameter (U2, U3, U, CU) - perhaps we should skip it?
| ], | ||
| 2, | ||
| ), | ||
| // iSwap, XXMinusYY, XXPlusYY: [XX, YY] |
There was a problem hiding this comment.
we should be more careful with XXMinusYY, XXPlusYY since they have 2 parameters (theta, beta).
If beta=0, then it seems to be correct.
otherwise, I'm not sure what happens. Perhaps we should skip it?
| StandardGate::CH => ( | ||
| &[ | ||
| &[(0, BitTerm::Z), (1, BitTerm::X)], | ||
| &[(0, BitTerm::Z), (1, BitTerm::Z)], |
There was a problem hiding this comment.
I don't think it is correct.
|
Hi @debasmita2102. I think it would be very useful to expose the For context, this method computes the generator of a standard gate and returns it as a In terms of the implementation details, I think it makes sense to move the You should do the other standard stuff, like adding pub fn standard_generators(m: &Bound<PyModule>) -> PyResult<()> {
m.add_wrapped(wrap_pyfunction!(generator_observable))?;
Ok(())
}to add_submodule(m, ::qiskit_quantum_info::standard_generators::standard_generators, "standard_generators")?;to In any case, this allows to call your code from Python, in the following way: from qiskit._accelerate import standard_generators
gate_class = HGate
gen = standard_generators.generator_observable(gate_class._standard_gate)
print(gen)with the output being Note that this is currently slightly incorrect as the terms in the generator are missing the Copying @ShellyGarion's suggestion from an offline discussion, we would then be able to check the correctness of the generator evo = PauliEvolutionGate(gen, time=1)
ok = Operator(gate_class()).equiv(Operator(evo)) # check equality up to the global phaseDoes this make sense? |
| &[ | ||
| &[(0, BitTerm::Z), (1, BitTerm::X)], | ||
| &[(1, BitTerm::X)], | ||
| &[(0, BitTerm::Z)], |
There was a problem hiding this comment.
Following @alexanderivrii comment, I think we should be more careful and add signs here, if we want the generators to be equivalent to the corresponsing gate (otherwise, commutativity may fail for higher order Pauli terms).
e.g., for CX gate:
op1 = SparsePauliOp('XZ')-SparsePauliOp('XI')-SparsePauliOp('IZ')
ev1 = PauliEvolutionGate(op1, time=np.pi/4)
print (Operator(CXGate()).equiv(Operator(ev1))) #= True
There was a problem hiding this comment.
Adding more gates equivalences:
op1 = SparsePauliOp('XZ')-SparsePauliOp('XI')-SparsePauliOp('IZ') // CX and CSX
op2 = SparsePauliOp('YZ')-SparsePauliOp('YI')-SparsePauliOp('IZ') // CY
op3 = SparsePauliOp('ZZ')-SparsePauliOp('ZI')-SparsePauliOp('IZ') // CZ and CS and CSdg
e_CY = PauliEvolutionGate(op2, time=np.pi/4)
e_CZ = PauliEvolutionGate(op3, time=np.pi/4)
e_CSX = PauliEvolutionGate(op1, time=-np.pi/8)
e_CS = PauliEvolutionGate(op3, time=-np.pi/8)
e_CSdg = PauliEvolutionGate(op3, time=np.pi/8)
print (Operator(CYGate()).equiv(Operator(e_CY)))
print (Operator(CZGate()).equiv(Operator(e_CZ)))
print (Operator(CSXGate()).equiv(Operator(e_CSX)))
print (Operator(CSGate()).equiv(Operator(e_CS)))
print (Operator(CSdgGate()).equiv(Operator(e_CSdg)))
op0 = SparsePauliOp('IX')-SparsePauliOp('XY') // ECR
op1 = SparsePauliOp('XX')+SparsePauliOp('YY')+SparsePauliOp('ZZ') // Swap
op2 = SparsePauliOp('XX')+SparsePauliOp('YY') // iSwap
e_ECR = PauliEvolutionGate(op0, time=np.pi/(2*np.sqrt(2)))
e_Swap = PauliEvolutionGate(op1, time=np.pi/4)
e_iSwap = PauliEvolutionGate(op2, time=-np.pi/4)
print (Operator(ECRGate()).equiv(Operator(e_ECR)))
print (Operator(SwapGate()).equiv(Operator(e_Swap)))
print (Operator(iSwapGate()).equiv(Operator(e_iSwap)))
There was a problem hiding this comment.
More equivalences for the parametric gates - note that there should be a factor that depends of the angle ("t"):
op1 = -t/4 * SparsePauliOp('XZ') + t/4 * SparsePauliOp('XI') # CRX
op2 = - t/4 * SparsePauliOp('YZ') + t/4 * SparsePauliOp('YI') # CRY
op3 = - t/4 * SparsePauliOp('ZZ') + t/4 * SparsePauliOp('ZI') # CRZ
op4 = - t/4 * SparsePauliOp('ZZ') + t/4 * SparsePauliOp('ZI') + t/4 * SparsePauliOp('IZ') # CPhase
e_CRX = PauliEvolutionGate(op1, time=1)
e_CRY = PauliEvolutionGate(op2, time=1)
e_CRZ = PauliEvolutionGate(op3, time=1)
e_CP = PauliEvolutionGate(operator=op4, time=1)
print (Operator(CRXGate(t)).equiv(Operator(e_CRX)))
print (Operator(CRYGate(t)).equiv(Operator(e_CRY)))
print (Operator(CRZGate(t)).equiv(Operator(e_CRZ)))
print (Operator(CPhaseGate(t)).equiv(Operator(e_CP)))
op_plus = t/4 * SparsePauliOp('XX') + t/4 * SparsePauliOp('YY') # XXPlusYY with beta=0
op_minus = t/4 * SparsePauliOp('XX') - t/4 * SparsePauliOp('YY') # XXMinusYY with beta=0
e_plus = PauliEvolutionGate(operator=op_plus, time=1)
e_minus = PauliEvolutionGate(operator=op_minus, time=1)
print (Operator(XXPlusYYGate(t, 0)).equiv(Operator(e_plus)))
print (Operator(XXMinusYYGate(t, 0)).equiv(Operator(e_minus)))
There was a problem hiding this comment.
btw, for H the equivalence is:
opH = SparsePauliOp('X') + SparsePauliOp('Z')
eH = PauliEvolutionGate(opH, time=np.pi/(2 * np.sqrt(2)))
print (Operator(HGate()).equiv(Operator(eH)))
For all gates that are equivalent Pauli rotation gates (i.e. have a single Pauli term), it's the same equvialence as in #15502
5a6edc1 to
3d6a203
Compare
ShellyGarion
left a comment
There was a problem hiding this comment.
This PR is almost ready now, only a few more comments.
| // `i32`, we use `i64`, but Scipy will always try to downcast to `i32`, so we try to match it. | ||
| let max_entries_per_row = (paulis.num_ops() as u64).min(1u64 << (paulis.num_qubits() - 1)); | ||
| let max_entries_per_row = | ||
| (paulis.num_ops() as u64).min(1u64 << paulis.num_qubits().saturating_sub(1)); |
There was a problem hiding this comment.
is there a reason for the change in this line?
There was a problem hiding this comment.
The switch to saturating_sub(1) I thought for a safety fix for when an operator has no qubits (like a standalone global phase). Previously, the code used (num_qubits - 1) , which can cause a crash in Rust if the count is 0. By using saturating_sub(1), it just returns 0 . I locally tested this with a zero-qubit scalar operator and it works smoothly.4
Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
ShellyGarion
left a comment
There was a problem hiding this comment.
I think the file "bicycle-architecture-compiler" does not belong to this PR
ShellyGarion
left a comment
There was a problem hiding this comment.
LGTM. Thank you very much @debasmita2102 for all the effort!
I'll wait with approving and merging this PR to see if @alexanderivrii or @Cryoris have any further comments.
|
Was any of this code generated by an LLM? There are multiple definitions of the same If so: this very much needs an LLM attribution prominently in the PR comment, and all the code blocks that were generated by LLM need attribution to the LLM and the model/version used. I think it might be a good idea to revert this and fix it up a bit more before re-merging. I'd rather the LLM attributions go in in exactly the same commit, so it's easier to trace through the |
…ard gates (Qiskit#15488)" This reverts commit 3942c9c.
This is a staging commit, splitting up the giant monolithic `qiskit_circuit::operations` to put `StandardGate` in its own module. The code is unchanged, other than imports. This is preparatory for moving more code out of `quantum_info` that connects `StandardGate` to base `quantum-info` objects. Unfortunately I couldn't make this register in `git` as a giant move, so the best way to review this commit is to verify that the diff of the moved block (lines 1163 to 3084) in `crates/circuit/src/operations.rs` in the parent of this commit[^1] is minimal. For example: ```bash ( git show 7ceda63:crates/circuit/src/operations.rs | head -n 3084 | tail -n+1163 ) | diff - crates/circuit/src/standard_gate/mod.rs ``` shows ```text 0a1,27 > // This code is part of Qiskit. > // > // (C) Copyright IBM 2026 > // > // This code is licensed under the Apache License, Version 2.0. You may > // obtain a copy of this license in the LICENSE.txt file in the root directory > // of this source tree or at https://www.apache.org/licenses/LICENSE-2.0. > // > // Any modifications or derivative works of this code must retain this > // copyright notice, and modified files need to carry a notice indicating > // that they have been altered from the originals. > > use crate::circuit_data::{CircuitData, PyCircuitData}; > use crate::operations::{Operation, Param, add_param, clone_param, multiply_param, radd_param}; > use crate::{Qubit, gate_matrix, impl_intopyobject_for_copy_pyclass, imports}; > > use ndarray::{Array2, aview2}; > use num_complex::Complex64; > use smallvec::{SmallVec, smallvec}; > use std::f64::consts::PI; > > use numpy::{IntoPyArray, PyArray2}; > use pyo3::prelude::*; > use pyo3::types::{IntoPyDict, PyList, PyTuple}; > > const FLOAT_ZERO: Param = Param::Float(0.0); > 1920,1922d1946 < < const FLOAT_ZERO: Param = Param::Float(0.0); < ``` [^1]: 7ceda63: Revert "Support commutation check between Pauli-based gates and standard gates (Qiskit#15488)" (Qiskit#15906)
|
@jakelishman Thanks for this details, yes, I was experimenting with GitHub Copilot (using the Claude 4.5 Haiku model via VS Code) with some review comments and in the process, I missed a few 'internal thought' comments and redundant function wrappers in place. Cleaned it up. Also mentioned the AI attribution in those files. For example What are the steps next? Will it be a new PR? The reason I understand (attribution about AI will remain in the same commit). |
This is a staging commit, splitting up the giant monolithic `qiskit_circuit::operations` to put `StandardGate` in its own module. The code is unchanged, other than imports. This is preparatory for moving more code out of `quantum_info` that connects `StandardGate` to base `quantum-info` objects. Unfortunately I couldn't make this register in `git` as a giant move, so the best way to review this commit is to verify that the diff of the moved block (lines 1163 to 3084) in `crates/circuit/src/operations.rs` in the parent of this commit[^1] is minimal. For example: ```bash ( git show 7ceda63:crates/circuit/src/operations.rs | head -n 3084 | tail -n+1163 ) | diff - crates/circuit/src/standard_gate/mod.rs ``` shows ```text 0a1,27 > // This code is part of Qiskit. > // > // (C) Copyright IBM 2026 > // > // This code is licensed under the Apache License, Version 2.0. You may > // obtain a copy of this license in the LICENSE.txt file in the root directory > // of this source tree or at https://www.apache.org/licenses/LICENSE-2.0. > // > // Any modifications or derivative works of this code must retain this > // copyright notice, and modified files need to carry a notice indicating > // that they have been altered from the originals. > > use crate::circuit_data::{CircuitData, PyCircuitData}; > use crate::operations::{Operation, Param, add_param, clone_param, multiply_param, radd_param}; > use crate::{Qubit, gate_matrix, impl_intopyobject_for_copy_pyclass, imports}; > > use ndarray::{Array2, aview2}; > use num_complex::Complex64; > use smallvec::{SmallVec, smallvec}; > use std::f64::consts::PI; > > use numpy::{IntoPyArray, PyArray2}; > use pyo3::prelude::*; > use pyo3::types::{IntoPyDict, PyList, PyTuple}; > > const FLOAT_ZERO: Param = Param::Float(0.0); > 1920,1922d1946 < < const FLOAT_ZERO: Param = Param::Float(0.0); < ``` [^1]: 7ceda63: Revert "Support commutation check between Pauli-based gates and standard gates (Qiskit#15488)" (Qiskit#15906)
This is a staging commit, splitting up the giant monolithic `qiskit_circuit::operations` to put `StandardGate` in its own module. The code is unchanged, other than imports. This is preparatory for moving more code out of `quantum_info` that connects `StandardGate` to base `quantum-info` objects. Unfortunately I couldn't make this register in `git` as a giant move, so the best way to review this commit is to verify that the diff of the moved block (lines 1163 to 3084) in `crates/circuit/src/operations.rs` in the parent of this commit[^1] is minimal. For example: ```bash ( git show 7ceda63:crates/circuit/src/operations.rs | head -n 3084 | tail -n+1163 ) | diff - crates/circuit/src/standard_gate/mod.rs ``` shows ```text 0a1,27 > // This code is part of Qiskit. > // > // (C) Copyright IBM 2026 > // > // This code is licensed under the Apache License, Version 2.0. You may > // obtain a copy of this license in the LICENSE.txt file in the root directory > // of this source tree or at https://www.apache.org/licenses/LICENSE-2.0. > // > // Any modifications or derivative works of this code must retain this > // copyright notice, and modified files need to carry a notice indicating > // that they have been altered from the originals. > > use crate::circuit_data::{CircuitData, PyCircuitData}; > use crate::operations::{Operation, Param, add_param, clone_param, multiply_param, radd_param}; > use crate::{Qubit, gate_matrix, impl_intopyobject_for_copy_pyclass, imports}; > > use ndarray::{Array2, aview2}; > use num_complex::Complex64; > use smallvec::{SmallVec, smallvec}; > use std::f64::consts::PI; > > use numpy::{IntoPyArray, PyArray2}; > use pyo3::prelude::*; > use pyo3::types::{IntoPyDict, PyList, PyTuple}; > > const FLOAT_ZERO: Param = Param::Float(0.0); > 1920,1922d1946 < < const FLOAT_ZERO: Param = Param::Float(0.0); < ``` [^1]: 7ceda63: Revert "Support commutation check between Pauli-based gates and standard gates (#15488)" (#15906)
…s (After reverting #15488) (#15925) * Transpiler: implement exact Pauli generators for LightCone in Rust This PR adds exact Pauli generator expansions for the following gates: - single-qubit: H (X+Z) - 2-qubit: CX, CY, CZ, ECR, Swap, iSwap, CSX, CS, CSdg - parametric: CRX, CRY, CRZ, CPhase, RXX, RYY, RZZ, RZX, XXPlusYY, XXMinusYY - 3-qubit: CCX, CSwap This enables high-performance bitwise commutation checks in the LightCone pass, avoiding slow matrix-multiplication fallbacks. Parametric gates are scaled dynamically in Rust using the input parameters. Co-authored-by: Shelly Garion <shelly.garion@ibm.com> Co-authored-by: Alexander Ivrii <alexander.ivrii@ibm.com> * Lint: apply cargo fmt and fix clippy::map_clone in split_2q_unitaries.rs * Expose generator_observable to Python Following feedback from Alexander Ivrii, this commit exposes the Rust generator_observable function and the StandardGate enum to Python under the qiskit._accelerate.standard_generators submodule. This allows developers to verify the mathematical generators directly from Python and reuse the logic for other purposes like PauliEvolutionGate verification. The Rust-internal API is maintained via a dedicated generator_observable function that accepts slices. * Refactor generator logic to use qiskit_circuit::StandardGate * Update generator_observable with expansions for CCX/CCZ/CSwap * Fix generator fallback for unsupported gates and update docs * Narrow PR scope: remove split_2q_unitaries and pyext python wrapper * Add generator support for CS, CSdg, CSX, CCX, CCZ; add commutation tests * style: run black on test_commutation_checker.py * style: run rustfmt on standard_generators.rs * lint: fix license headers and pylint warnings * style: remove decorative unicode lines and add operator equivalence tests * fix: ensure strict Operator equivalence for all supported generators * style: fix pylint reimport and missing variables in test_commutation_checker.py * Add generic generator equivalence test, fix matrix conversion, and optimize gate generators * Format python tests * Update copyright year in standard_generators.rs * Clarify docstring for multi-qubit generators context * Fix XXPlusYY and XXMinusYY generator validation and test cases * Move test imports to top of file * Use ddt data parameterization for gate generator testing tests * Refactor test_clifford_gates_have_generators to use ddt * Add CPhase, CRX, CRY, CRZ to generator tests * Update test_all_gates_operator_equivalence_ddt docstring to be more accurate * Move all inline imports to the top of the file * Reorganize test data to group related gates logically * Add tests for unsupported gates and parameters in generator logic * Rename test helper to _commute to reflect that it uses either generator or matrix paths * Clarify parameter handling in generator_observable docstring * Comprehensive audit: Add Identity support and register all missing gates in SUPPORTED_OP * Fix: Correct gate generator expansions and resolve UnsortedIndices layout panic * pose generator engine, restore overflow logic, expand tests, and centralize constants * Update crates/circuit/src/util.rs Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> * Update releasenotes/notes/pauli-evolution-commutation-8f7caab8.yaml Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> * Update releasenotes/notes/pauli-evolution-commutation-8f7caab8.yaml Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> * added missing gate, added test, copyright year, doc explaining * restore negative constants to util.rs for generator expansions * fix lint errors in regression test and move imports to top * Final PR fixes: address remaining lint issues and update copyright headers * Remove accidental temporary files from PR * Restore original copyright years for existing files (only PR-created files keep 2026) * Permanently delete bicycle-architecture-compiler from branch * resolved merge conflicts * removed redundant constants blocking branch conflict * fix lint and formatting issues * docstring edits * docstring formatting * fixing formatting * revert docstring changes to fix lint failure * Reverted changes in max_entries_per_row * renaming tests * readded the comments in cargo test * Cleanup and reword method - Test all standard gate matrices - Enable the existing LightCone pass instead of adding a new one - Removed LLM-supported tests in commutation checker in favor of human-only ones - Rename to standard gate exponent - Simplify match statements - Rm duplicate function definitions and complex number crate - Use local consts over pub consts - Reword reno * Reno and rm duplicated block * Sasha's review comments - reword exponent docs - cleanup CC tests - typos And rm LightCone runtime warning which no longer applies * Remove some unused imports * Move pickle import back to where it was * Fix circuit crate dependency * Rm incorrect sentence on commutation --------- Co-authored-by: Shelly Garion <shelly.garion@ibm.com> Co-authored-by: Alexander Ivrii <alexander.ivrii@ibm.com> Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
Summary
try_pauli_generatornow extractsSparseObservablefromPauliEvolutionGate._extract_sparse_observable()