Use average gate fidelity in the commutation checker #13874

Cryoris · 2025-02-18T17:36:40Z

Summary

The commutation checker currently uses a different metric to check when gates commute than other places in Qiskit, such as the RemoveIdentityEquivalent. This leads to strange behavior, where one gate is treated as identity in the commutation, but is subsequently not removed as other passes do not treat it as identity. This is what causes #13547.

This PR updates the commutation checker to use the same average gate fidelity and thus the transpiler to handle almost-identity gates more consistently.

Closes #13547.

Details and comments

Still needs a reno.
This has one small caveat: the definition of the average gate fidelity is global phase agnostic, but our commutation rules are not. I.e. we currently do not want to commute two gates, if doing that introduces a global phase. We could extend this in the future, but this is outside the scope of this PR.
This PR increases the threshold for gates to commute (== rotation angles must now be smaller than before in order to commute). This means the #gates we transpile a QFT to increases, see the benchmark below. However, @ElePT ran benchpress and this didn't seem to cause a noticable regression overall.

Here's a small benchmark comparing transpile time, CX depth and count for the (1) the QFT, (2) an efficient SU2 circuit with log(num qubits) depth and (3) a Heisenberg Trotter evolution on a line.

…kit#13417)" This reverts commit b9d5c9c.

qiskit-bot · 2025-02-18T17:36:46Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core

Cryoris · 2025-02-24T12:37:55Z

This currently fails because the error tolerance for the matrix-based checks is too low. This leads e.g. to the matrix-based check of [H, H] to return False. For this to be more robust we'd have to increase the check threshold, the question is whether to just use some const * eps or whether to base it on the matrix dimension.

these cover cases where we accumulate roundoff errors

- increased the threshold for value comparisons to 16 EPS - use same functions in RemoveIdentityEquiv and CC checker - add reno - add detailed docs - add some tests for above thresholds

coveralls · 2025-03-03T10:02:37Z

Pull Request Test Coverage Report for Build 13687167568

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

142 of 148 (95.95%) changed or added relevant lines in 7 files are covered.
260 unchanged lines in 24 files lost coverage.
Overall coverage decreased (-0.2%) to 87.002%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/gate_metrics.rs	33	34	97.06%
crates/accelerate/src/commutation_checker.rs	64	66	96.97%
crates/accelerate/src/remove_identity_equiv.rs	16	19	84.21%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_compose.rs	1	76.09%
crates/accelerate/src/unitary_synthesis.rs	1	94.39%
crates/qasm2/src/expr.rs	1	94.23%
qiskit/primitives/utils.py	1	92.86%
qiskit/result/distributions/quasi.py	1	95.92%
qiskit/result/sampled_expval.py	2	93.55%
qiskit/transpiler/passes/optimization/template_matching/template_substitution.py	2	93.29%
qiskit/utils/units.py	3	90.91%
qiskit/primitives/base/base_sampler.py	5	79.17%
qiskit/circuit/parameterexpression.py	6	97.68%

Totals
Change from base Build 13673594587:	-0.2%
Covered Lines:	76053
Relevant Lines:	87415

💛 - Coveralls

which is consistent with the 2qWeylDecomp and still stricter than Qiskit 1.x

mtreinish

Overall this LGTM, it'll be good to have everything consistent. Just a few inline comments.

mtreinish · 2025-03-05T16:05:00Z

crates/accelerate/src/commutation_checker.rs

+        // If the average gate infidelity is below this tolerance, they commute. The tolerance
+        // is set to max(1e-12, 1 - approximation_degree), to account for roundoffs and for
+        // consistency with other places in Qiskit.
+        let tol = 1e-12_f64.max(1. - approximation_degree);


This does mean we can't approximate more than what a 1e-12 tolerance provides. But it's the same logic we use in other places so I think that's fine.

Yeah it is unfortunately. I don't like this very much either, but currently approximation_degree is 1. everywhere per default which doesn't really allow us to set another default like 1-1e-12. The other option would've been to use None as indicator, but that already means that the target error rates should be used.

mtreinish · 2025-03-05T16:12:23Z

crates/accelerate/src/commutation_checker.rs

+    let dim = match rotation {
+        "rx" | "ry" | "rz" | "p" | "u1" => 2.,
+        _ => 4.,
+    };
+
+    let trace_over_dim = match rotation {
+        "rx" | "ry" | "rz" | "rxx" | "ryy" | "rzx" | "rzz" => {
+            Complex64::new((angle / 2.).cos(), 0.)
+        }
+        "crx" | "cry" | "crz" => Complex64::new(0.5 + 0.5 * (angle / 2.).cos(), 0.),
+        "p" | "u1" => (1. + Complex64::new(0., angle).exp()) / 2.,
+        "cp" => (3. + Complex64::new(0., angle).exp()) / 4.,
+        _ => return None,


I feel like we could do this without the strings and used the StandardGate enum variants from standard gates. It would be faster to do the enum match because it's a u8 vs a string comparison.

I just saw this is used elsewhere in accelerate. I feel like this comment is especially true if we're going to use this for standardized trace computation for standard gates.

mtreinish · 2025-03-05T16:15:13Z

crates/accelerate/src/remove_identity_equiv.rs

@@ -14,6 +14,7 @@ use num_complex::ComplexFloat;
 use pyo3::prelude::*;
 use rustworkx_core::petgraph::stable_graph::NodeIndex;

+use crate::commutation_checker::rotation_trace_and_dim;


Do you think we should put this somewhere more central?

Yeah I was looking for a more central place but didn't find an existing one that suited well.. and I didn't want to create a new one just with this file. Maybe we can just create a gate_metrics.rs or something that also includes the gate_fidelity function

mtreinish · 2025-03-05T16:17:22Z

crates/accelerate/src/remove_identity_equiv.rs

@@ -33,12 +34,15 @@ fn remove_identity_equiv(
 ) {
    let mut remove_list: Vec<NodeIndex> = Vec::new();
    let mut global_phase_update: f64 = 0.;
+    // Minimum threshold to compare average gate fidelity to 1. This is chosen to account
+    // for roundoff errors and to be consistent with other places.
+    let minimum_tol = 1e-12_f64;


I'd make this a const:

Suggested change

let minimum_tol = 1e-12_f64;

const MINIMUM_TOL: f64 = 1e-12;

Although you might have to move it outside the function definition to do this.

mtreinish · 2025-03-05T16:19:32Z

crates/accelerate/src/remove_identity_equiv.rs

+                        let name = gate.name();
+                        if let Param::Float(angle) = inst.params_view()[0] {
+                            let (tr_over_dim, dim) =
+                                rotation_trace_and_dim(name, angle).expect("All rotation covered");


Since this becomes a panic message maybe something a bit more descriptive/clear. We should never encounter it because the match set is complete but since you went to the trouble of adding a message instead of just unwrap() we should provide a bit more detail.

mtreinish · 2025-03-05T16:22:58Z

crates/accelerate/src/unitary_compose.rs

+    let mut out = arr2(&[
+        [C_ZERO, C_ZERO, C_ZERO, C_ZERO],
+        [C_ZERO, C_ZERO, C_ZERO, C_ZERO],
+        [C_ZERO, C_ZERO, C_ZERO, C_ZERO],
+        [C_ZERO, C_ZERO, C_ZERO, C_ZERO],
+    ]);


Is there a reason to not do?:

Suggested change

let mut out = arr2(&[

[C_ZERO, C_ZERO, C_ZERO, C_ZERO],

[C_ZERO, C_ZERO, C_ZERO, C_ZERO],

[C_ZERO, C_ZERO, C_ZERO, C_ZERO],

[C_ZERO, C_ZERO, C_ZERO, C_ZERO],

]);

let mut out = Array2::zeros((4, 4));

There's zero reason not to 😉

mtreinish · 2025-03-05T16:23:46Z

crates/accelerate/src/unitary_compose.rs

-///
-/// This is analogous to NumPy's ``allclose`` function.
-pub fn allclose(
+pub fn mm2q(


Maybe write this out as matmul_2q or something a bit more descriptive? It took me a sec to understand what this meant at first. Or just a docstring to say what it is.

mtreinish · 2025-03-05T16:26:16Z

crates/accelerate/src/unitary_compose.rs

-        }
-    }
-    true
+fn mm1q(left: &ArrayView2<Complex64>, right: &ArrayView2<Complex64>) -> Array2<Complex64> {


Maybe write this out as matmul_1q or something a bit more descriptive? It took me a sec to understand what this meant at first then I looked at the code. Or just a docstring to say what it is.

There is also an existing function for this here: https://github.com/Qiskit/qiskit/blob/main/crates/accelerate/src/euler_one_qubit_decomposer.rs#L1233-L1245 but it works with slightly different types.

mtreinish · 2025-03-05T16:27:13Z

qiskit/circuit/__init__.py

-.. autosummary::
-   :toctree: ../stubs/
-
-   CommutationChecker


Isn't the CommutationChecker part of the public API we probably shouldn't remove this. (we had this exact problem as part of 1.1 or something where somebody removed it from the docs)

huh the page doesn't exist on the 1.4 docs (which is probably why I removed it here), but it does exist on the 2.0 dev doc -- I'll add it again, thanks for the check!

mtreinish · 2025-03-05T16:28:15Z

releasenotes/notes/cc-gate-fidelity-bde7a01dc8c56e29.yaml

@@ -0,0 +1,24 @@
+---
+upgrade:


You need a feature note about the new argument on the commutative pass constructors.

in my mind it was not API documented 😛 I'll add it!

mtreinish

Thanks for the quick updates. There are 3 small nits inline otherwise I think this is ready to go. I left one larger comment inline about to rewrite the rotation gate handling without using a string. But we can save that for a follow up, it's just a pet peeve of mine and is arguably scope creep for this PR.

mtreinish · 2025-03-05T22:15:21Z

crates/accelerate/src/gate_metrics.rs

+    let dim = left.nrows(); // == left.ncols() == right.nrows() == right.ncols()
+                            // let trace = left.t().mapv(|el| el.conj()).dot(right).diag().sum();


Are these code comments intentional?

The first lines yes, the second no 😄 but also the first line is not really necessary, I removed them both

crates/accelerate/src/unitary_compose.rs

mtreinish · 2025-03-05T22:33:15Z

crates/accelerate/src/commutation_checker.rs

@@ -652,13 +605,19 @@ fn map_rotation<'a>(
 ) -> (&'a OperationRef<'a>, &'a [Param], bool) {
    let name = op.name();

-    if let Some((pi_multiple, generator)) = SUPPORTED_ROTATIONS.get(name) {
+    if let Some(generator) = SUPPORTED_ROTATIONS.get(name) {


Maybe not for this PR but we can drop the hashset here all together and have a static lookup table based on the standard gates. Something like:

const fn builld_lut() -> [Option<StandardGate>; STANDARD_GATE_SIZE] { ... } let static supported_rotations: [Option<StandardGate>; STANDARD_GATE_SIZE] = build_lut()

Alternatively you could just do a couple of if matches!() on .standard_gate().

When in rust space and working solely with standard gates we really never have a need for strings, so this just sticks out to me every time I see it. But it was pre-existing so we can do this in a follow up.

Yeah that sounds like a good idea, I'll put it on the follow-up list if that's good for you 🙂

Cryoris added 14 commits January 10, 2025 17:02

v0 of comm checker

125c9e8

dangling print

ba5d37d

Merge branch 'main' into gate-fidelity-everywhere

92ebe54

games with matrices

501c808

Revert "HLS fix to not synthesize instructions already supported (Qis…

7d9fdb3

…kit#13417)" This reverts commit b9d5c9c.

Merge branch 'main' into gate-fidelity-everywhere

2edef3d

use custom mm1 and mm2

2697f9f

Merge branch 'main' of github.com:Qiskit/qiskit

cb03668

Merge branch 'main' into gate-fidelity-everywhere

0401aea

expose tol

38e04b3

rename tol to approximation_degree

c1efb19

note: this is not global phase correct!

3116483

don't be phase agnostic

c2d77a2

Merge branch 'main' into gate-fidelity-everywhere

005ec93

Cryoris requested a review from a team as a code owner February 18, 2025 17:36

Cryoris added this to the 2.0.0 milestone Feb 18, 2025

1ucian0 assigned mtreinish Feb 20, 2025

Cryoris added 4 commits February 24, 2025 14:06

Merge branch 'main' into gate-fidelity-everywhere

c6180da

additional tests

afc4ed4

these cover cases where we accumulate roundoff errors

Increase threshold, better consistency

adbd9be

- increased the threshold for value comparisons to 16 EPS - use same functions in RemoveIdentityEquiv and CC checker - add reno - add detailed docs - add some tests for above thresholds

Python-space approx degree oversight

cda06f8

update threshold to 1e-12

2c48ac4

which is consistent with the 2qWeylDecomp and still stricter than Qiskit 1.x

mtreinish reviewed Mar 5, 2025

View reviewed changes

mtreinish added the mod: transpiler Issues and PRs related to Transpiler label Mar 5, 2025

Cryoris added 2 commits March 5, 2025 17:45

Merge branch 'main' into gate-fidelity-everywhere

8f49c1c

review comments

6bc5656

mtreinish reviewed Mar 5, 2025

View reviewed changes

inline and rm comments

34e9fcc

mtreinish approved these changes Mar 5, 2025

View reviewed changes

mtreinish enabled auto-merge March 5, 2025 23:09

mtreinish added this pull request to the merge queue Mar 6, 2025

Merged via the queue into Qiskit:main with commit d2f4861 Mar 6, 2025
20 checks passed

Cryoris deleted the gate-fidelity-everywhere branch March 6, 2025 07:58

Cryoris mentioned this pull request Mar 6, 2025

Avoid string usage in CommutationChecker #13963

Open

mtreinish mentioned this pull request Mar 11, 2025

Overloaded gate names cause panic in commutation checker #13988

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use average gate fidelity in the commutation checker #13874

Use average gate fidelity in the commutation checker #13874

Cryoris commented Feb 18, 2025 •

edited

Loading

qiskit-bot commented Feb 18, 2025

Cryoris commented Feb 24, 2025

coveralls commented Mar 3, 2025 •

edited

Loading

mtreinish left a comment

mtreinish Mar 5, 2025

Cryoris Mar 5, 2025

mtreinish Mar 5, 2025

mtreinish Mar 5, 2025

mtreinish Mar 5, 2025

Cryoris Mar 5, 2025

mtreinish Mar 5, 2025

mtreinish Mar 5, 2025

mtreinish Mar 5, 2025

Cryoris Mar 5, 2025

mtreinish Mar 5, 2025

mtreinish Mar 5, 2025

mtreinish Mar 5, 2025

Cryoris Mar 5, 2025

mtreinish Mar 5, 2025

Cryoris Mar 5, 2025

mtreinish left a comment

mtreinish Mar 5, 2025

Cryoris Mar 5, 2025

mtreinish Mar 5, 2025

Cryoris Mar 5, 2025

		let dim = left.nrows(); // == left.ncols() == right.nrows() == right.ncols()
		// let trace = left.t().mapv(\|el\| el.conj()).dot(right).diag().sum();

		@@ -0,0 +1,24 @@
		---
		upgrade:

Use average gate fidelity in the commutation checker #13874

Use average gate fidelity in the commutation checker #13874

Conversation

Cryoris commented Feb 18, 2025 • edited Loading

Summary

Details and comments

qiskit-bot commented Feb 18, 2025

Cryoris commented Feb 24, 2025

coveralls commented Mar 3, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13687167568

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cryoris commented Feb 18, 2025 •

edited

Loading

coveralls commented Mar 3, 2025 •

edited

Loading