More conservative caching in the `CommutationChecker` #13600

Cryoris · 2024-12-23T15:00:41Z

Summary

This PR might require some discussion on what assumptions we have on custom Instructions. However, if we cannot assume that an instruction's definition is fully specified by it's params, then we cannot use the params as key in the commutation library. (We also sometimes put additional information outside of params.)
This PR makes the commutation cache more conservative, by only additionally caching standard gate relations, where we know the instructions are defined by it's params.

Details and comments

The commutation checker currently caches commutations of any instruction with float-only params (with some hardcoded exceptions). This leads to problems if the instruction's definition depends on more than just the params, e.g. we could have a custom gate defined as

class MyEvilRXGate(Gate):
    """A RX gate designed to annoy the caching mechanism (but a realistic gate nevertheless)."""

    def __init__(self, evil_input_not_in_param: float):
        """
        Args:
            evil_input_not_in_param: The RX rotation angle.
        """
        self.value = evil_input_not_in_param
        super().__init__("<evil laugh here>", 1, [])

    def _define(self):
        self.definition = QuantumCircuit(1)
        self.definition.rx(self.value, 0)

Calling the commutation checker with an instance of this gate will then cache the first commutation, under the key [] (since the params are empty) and subsequent queries of the cache return the first commutation, regardless of the evil rotation angle. For example

all_commuter = MyEvilRXGate(0)  # this will with anything
some_rx = MyEvilRXGate(1.6192)  # this should not commute with H

print(scc.commute(all_commuter, [0], [], HGate(), [0], []))  # True, as we expect
# this should be False, but queries the first occurence of MyEvilRXGate(0) and is True!
print(scc.commute(some_rx, [0], [], HGate(), [0], []))

Another solution could be to explicitly state the assumption that the gate must be fully specified by the params attribute, and we'd have to update some gates in the library.

I ran the test/benchmarks/utility_scale.py benchmarks and could not see a regression on my laptop.

qiskit-bot · 2024-12-23T15:00:45Z

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

@Qiskit/terra-core

coveralls · 2024-12-23T15:26:07Z

Pull Request Test Coverage Report for Build 12652621375

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

10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
121 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.01%) to 88.939%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.73%
qiskit/primitives/containers/observables_array.py	5	95.0%
crates/qasm2/src/parse.rs	12	96.69%
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py	20	94.58%
crates/accelerate/src/unitary_synthesis.rs	23	93.18%
qiskit/transpiler/passes/synthesis/hls_plugins.py	58	83.38%

Totals
Change from base Build 12420636821:	-0.01%
Covered Lines:	79446
Relevant Lines:	89326

💛 - Coveralls

alexanderivrii · 2024-12-29T12:35:21Z

Thanks @Cryoris, I agree that it's better to not cache a commutation relationship rather than to cache a wrong one. Nevertheless, I thought we had a way both to check and to store that $RZ(\theta)(0)$ commutes with $CX(0, 1)$ for every possible value of $\theta$, including the parametric case -- do I remember this correctly?

Cryoris · 2025-01-06T09:34:38Z

Yes, we cover the parametric $RZ(\theta)$ case -- but only for the Qiskit-native RZGate. This PR disables params-based caching for any non-native gate, since we cannot be certain that the gate instances only differ in the params attribute.

We can also discuss this with the team, whether this is the right approach or we should rather make the assumption that gate instances do only differ by their params (which currently is not the case for some library gates). Personally, I would asusme that the commutation checker by far works on Qiskit-native gates so using this more conservative approach likely doesn't affect performance and is safer 🙂

alexanderivrii

I think that caching/checking commutation relations for standard gates only makes a lot of sense. I had a few in-line questions (probably showing that I have not fully followed the recent updates to commutation checking). One other question, if I remember correctly, at some point @brandhsn has generated a database of commutation relations, are we still using it, and if so, should it be regenerated for standard gates only?

I agree that a more long-term plan might be to rethink the role of params when defining custom gates, but let's leave this as a possible followup.

alexanderivrii · 2025-01-07T11:52:46Z

test/python/circuit/test_commutation_checker.py

-        self.assertTrue(scc.commute(NewGateCX(), [0, 2], [], CXGate(), [0, 1], []))
-        self.assertFalse(scc.commute(NewGateCX(), [0, 1], [], CXGate(), [1, 2], []))
-        self.assertTrue(scc.commute(NewGateCX(), [1, 4], [], CXGate(), [1, 6], []))
-        self.assertFalse(scc.commute(NewGateCX(), [5, 3], [], CXGate(), [3, 1], []))
+        cx_like = CUGate(np.pi, 0, np.pi, 0)
+        self.assertTrue(scc.commute(cx_like, [0, 2], [], CXGate(), [0, 1], []))
+        self.assertFalse(scc.commute(cx_like, [0, 1], [], CXGate(), [1, 2], []))
+        self.assertTrue(scc.commute(cx_like, [1, 4], [], CXGate(), [1, 6], []))
+        self.assertFalse(scc.commute(cx_like, [5, 3], [], CXGate(), [3, 1], []))


Please tell me if I understand correctly. NewGateCX is not a standard gate, so with this PR we will no longer cache its commutation relations. And cx_like is a standard gate (a CUGate). This is why it makes sense to update the tests to reason about cx_like instead of NewGateCX, correct? However, the scc.commute method should still be able to handle NewGateCX. So maybe it makes sense to leave a test that deals with custom gates. Do we still have such a test?

Correct to all of the above 👍🏻

We don't have a test using NewGateCX concretely, only with the EvilRXGate, which should test the same thing. I can add a test with NewGateCX though just to be safe.

Thanks, having an additional test for a benign custom gate makes sense.

Added in 44457f7 👍🏻

alexanderivrii · 2025-01-07T11:53:41Z

test/python/circuit/test_commutation_checker.py

+    def __init__(self, evil_input_not_in_param: float):
+        """
+        Args:
+            evil_input_not_in_param: The RX rotation angle.
+        """
+        self.value = evil_input_not_in_param
+        super().__init__("<evil laugh here>", 1, [])


I love this. This is what I meant earlier by saying that you are taking the art of writing tests to a whole new level 😄.

test/python/circuit/test_commutation_checker.py

alexanderivrii

LGTM, thanks for the fix and updating the tests.

Cryoris · 2025-01-16T17:45:36Z

@Mergifyio backport stable/1.3

mergify · 2025-01-16T17:45:40Z

backport stable/1.3

✅ Backports have been created

#13677 More conservative caching in the CommutationChecker (backport #13600) has been created for branch stable/1.3

* conservative commutation check * tests and reno * reno in the right location * more tests for custom gates (cherry picked from commit 93d796f)

) * conservative commutation check * tests and reno * reno in the right location * more tests for custom gates (cherry picked from commit 93d796f) Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>

Cryoris added 2 commits December 23, 2024 15:32

conservative commutation check

e383aa8

tests and reno

793e5ab

Cryoris added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Dec 23, 2024

Cryoris added this to the 1.3.2 milestone Dec 23, 2024

Cryoris requested a review from a team as a code owner December 23, 2024 15:00

reno in the right location

0072f91

alexanderivrii reviewed Jan 7, 2025

View reviewed changes

more tests for custom gates

44457f7

alexanderivrii approved these changes Jan 7, 2025

View reviewed changes

alexanderivrii added this pull request to the merge queue Jan 7, 2025

Merged via the queue into Qiskit:main with commit 93d796f Jan 7, 2025
17 checks passed

Cryoris deleted the conservative-commute branch January 16, 2025 17:45

mergify bot pushed a commit that referenced this pull request Jan 16, 2025

More conservative caching in the CommutationChecker (#13600)

7ea1f39

* conservative commutation check * tests and reno * reno in the right location * more tests for custom gates (cherry picked from commit 93d796f)

mergify bot mentioned this pull request Jan 16, 2025

More conservative caching in the CommutationChecker (backport #13600) #13677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More conservative caching in the `CommutationChecker` #13600

More conservative caching in the `CommutationChecker` #13600

Cryoris commented Dec 23, 2024 •

edited

Loading

qiskit-bot commented Dec 23, 2024

coveralls commented Dec 23, 2024 •

edited

Loading

alexanderivrii commented Dec 29, 2024

Cryoris commented Jan 6, 2025

alexanderivrii left a comment

alexanderivrii Jan 7, 2025

Cryoris Jan 7, 2025

alexanderivrii Jan 7, 2025

Cryoris Jan 7, 2025

alexanderivrii Jan 7, 2025

alexanderivrii left a comment

Cryoris commented Jan 16, 2025

mergify bot commented Jan 16, 2025 •

edited

Loading

More conservative caching in the CommutationChecker #13600

More conservative caching in the CommutationChecker #13600

Conversation

Cryoris commented Dec 23, 2024 • edited Loading

Summary

Details and comments

qiskit-bot commented Dec 23, 2024

coveralls commented Dec 23, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12652621375

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

alexanderivrii commented Dec 29, 2024

Cryoris commented Jan 6, 2025

alexanderivrii left a comment

Choose a reason for hiding this comment

alexanderivrii Jan 7, 2025

Choose a reason for hiding this comment

Cryoris Jan 7, 2025

Choose a reason for hiding this comment

alexanderivrii Jan 7, 2025

Choose a reason for hiding this comment

Cryoris Jan 7, 2025

Choose a reason for hiding this comment

alexanderivrii Jan 7, 2025

Choose a reason for hiding this comment

alexanderivrii left a comment

Choose a reason for hiding this comment

Cryoris commented Jan 16, 2025

mergify bot commented Jan 16, 2025 • edited Loading

✅ Backports have been created

More conservative caching in the `CommutationChecker` #13600

More conservative caching in the `CommutationChecker` #13600

Cryoris commented Dec 23, 2024 •

edited

Loading

coveralls commented Dec 23, 2024 •

edited

Loading

mergify bot commented Jan 16, 2025 •

edited

Loading