[bugfix] Prevent GateOperation.add from superseding Circuit.radd #7707
+45
−11
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 aPauliStringis 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 + PauliSumbecause the latter isn't anOperation. The simplest fix for this would be to addPauliSumto the new isinstance check, e.g.isinstance(other, (Operation, Number, PauliSum)), However that approach addsPauliSumas a dependency ofGateOperation, which seems unnatural. So instead, I added logic inPauliSum.__add__to convert ops toPauliStringfirst. 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 denylistCircuitinstead, 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 usedraddon operations in the same wayCircuitdoes.)Finally, I updated the
Circuit.raddunit test to include assertions that would have broken under the old code. The existing test usedXgates 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 gateHto 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 ofotherto which it can apply, as otherwise the change can break__radd__functionality in unrelated classes.Fixes #7706