-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix: Allow subtraction of IdentityGate in PauliSum.__sub__ #7276
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.
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
base: main
Are you sure you want to change the base?
Fix: Allow subtraction of IdentityGate in PauliSum.__sub__ #7276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, follows the existing pattern from the add method. Once you add unit tests for the test cases from the linked issue, should be good to merge.
f127113
to
31c8ae9
Compare
31c8ae9
to
6b9ec8b
Compare
Pushed the unit test from the linked issue |
for p2 in paulis: | ||
for op in (add, sub, addm): | ||
try: | ||
_ = op(p1, p2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I+I
and I-I
will still fail here, for reasons described in the linked issue thread. If so, probably have to add a conditional check here to skip those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. Surprisingly I+I
passes now even though it didn't in the linked issue and this PR didn't change __add__
. Any idea what changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My test wraps all gates in PauliString, so operations like PauliString(I(q)) + PauliString(I(q)) are handled correctly and the test passes. If you use raw I(q) + I(q) (without PauliString), it will fail, but that's not the intent of this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. But wouldn't those all also pass prior to your change? IIUC the change fixes unwrapped paulis by implicitly wrapping them during subtraction. So IIUC this test where they're explicitly being wrapped beforehand wouldn't actually test the fix. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No you were right. Those tests didn't test anything new. Just added new tests that should cover the changes. Let me know if that looks good.
In terms of the I+I, it's still not passing on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I+I
being out of scope was discussed in the linked issue. It might not be that hard to create an IdentityGate.__add__/__sub__
that replaces self
with an empty PauliString first (or maybe DensePauliString
since it's a gate?). Now that I think of it, that approach might even remove the need for these instance checks in PauliSum.__add__/__sub__
? Or it may not work at all and/or break some other tests. Haven't tried.
If you want to give that a shot and see if it works, go ahead. Otherwise, this looks good, just open up a separate issue to track the I+I
case and mention the issue number in your unit test's comment about bypassing the I+I
case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, check/format-incremental( --apply)
, check/pylint-changed-files
, check/pytest(-changed-files)
, check/mypy
, check/all
from the command line are useful for avoiding having to wait for CI just to find silly mistakes. https://github.com/quantumlib/Cirq/tree/main/check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks! Updated the new issue to #7280
…including identity cases (except I+I)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7276 +/- ##
==========================================
- Coverage 98.66% 98.66% -0.01%
==========================================
Files 1106 1106
Lines 96086 96097 +11
==========================================
+ Hits 94808 94818 +10
- Misses 1278 1279 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0340b0b
to
e860d08
Compare
…ty-bug' into fix-paulisum-identity-bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm cc @pavoljuhas
Summary
Fixes #7120
This PR fixes a bug where subtracting a GateOperation with an IdentityGate (e.g., cirq.I(q) - cirq.Z(q)) would raise a TypeError instead of returning a valid PauliSum.
Details
Updated PauliSum.sub in cirq-core/cirq/ops/linear_combinations.py to promote GateOperation(IdentityGate) to a PauliString() when performing subtraction.
This ensures arithmetic like cirq.I(q) - cirq.Z(q) now works as expected and matches the behavior for addition.
The fix addresses issue # (link to the original issue if possible).
Reproduction
Before this fix, the following would raise an error: