[bugfix] Prevent GateOperation.add from superseding Circuit.radd #7707
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per issue #7706, the change in #7651 caused
Circuit.__radd__
, which allows+
to prepend operations to circuits, e.g.H(q) + Circuit(...) == Circuit(H(q), ...)
, to stop working. The root cause is that the change added aGateOperation.__add__
, which thus supersedesCircuit.__radd__
and changes the behavior of+
in such cases.The fix for this is to modify the new
GateOperation.__add__
(andGateOperation.__sub__
, for symmetry) so that it only applies in cases where interpreting the sum as aPauliString
is reasonable.To do this, first, I updated
GateOperation.__add__
with a check,if not isinstance(other, (Operation, Number)): return NotImplemented
, since ops and numbers are the only things that can be interpreted as aPauliString
. This change preventsGateOperation.__add__
from interfering withCircuit.__radd__
, and constitutes the crux of the fix.That change in isolation breaks
GateOperation + PauliSum
because the latter isn't anOperation
. The simplest fix for this would be to addPauliSum
to the new isinstance check, e.g.isinstance(other, (Operation, Number, PauliSum))
, However that approach addsPauliSum
as a dependency ofGateOperation
, which seems unnatural. So instead, I added logic inPauliSum.__add__
to convert ops toPauliString
first. That approach doesn't create any new dependencies, and is also more robust because it works for all operations, not just GateOperations.(Note, an even simpler fix for the entire issue would have been, instead of allowlisting
(Operation, Number)
inGateOperation.__add__
, to denylistCircuit
instead, e.g.if isinstance(other, Circuit): return NotImplemented
. That would have fixed the issue and not required any special change forPauliSum
. But similarly, I chose against that route because of the unnatural dependency it would create, as well as the fact that that fix wouldn't help if any third-party classes usedradd
on operations in the same wayCircuit
does.)Finally, I updated the
Circuit.radd
unit test to include assertions that would have broken under the old code. The existing test usedX
gates everywhere, which didn't fail when prepending because of misc internal logic for Pauli gates. So I parameterized the test ongate
, and added a non-Pauli gateH
to the parameters, which will now fail if a similar regression is made in the future. I updated some tests in linear_combinations to test__sub__
more thoroughly as well, for symmetry.Key takeaway: be careful when implementing
__add__
for an existing class, and limit the scope ofother
to which it can apply, as otherwise the change can break__radd__
functionality in unrelated classes.Fixes #7706