-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Eliminate multiple control layers on CX/CZ.controlled([0]) #7241
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
Comments
csynque meeting - let us verify if this behavior was intended, ie, is expected elsewhere or should be changed as suggested. |
@pavoljuhas sgtm. Also note there's an ulterior motive here: if we can do this, #7242, and #7238, it'll allow us to get rid of this ugly block that special-cases controlled CZ gates: Cirq/cirq-core/cirq/ops/controlled_gate.py Lines 190 to 221 in 50d7198
It's not the end of the world if it can't be supported. But I tried to break that into three separate issues that each are improvements on existing behavior. There's precedent for a similar change here: #5883 (comment), with rationale for it being non-breaking. cc @tanujkhattar |
Fixes #7241 by absorbing the control layer of controlled-CZ into the control itself, leaving a controlled-Z, and does the equivalent for CX / X. As documented in the issue, this approach allows for more consistency in how controlled gates are represented. As stated in the linked issue, almost all existing controlled-CZ cases already work this way; the only outlier is when control_values==[0]. Eliminating that outlier allows most of the special handling to be consolidated in the base gates (Z and X), so it's already possible to see some benefit from the added consistency.
Uh oh!
There was an error while loading. Please reload this page.
Describe your design idea/issue
All other cases of
.controlled()
in Cirq, as well as theControlledGate
constructor, do their best to flatten the layers of control where possible. The one exception isCX/CZ.controlled(...)
, where...
is a non-default set of control values.Output:
The
CX
andCZ
in the nontrivial control value cases end up with ControlledGates that wrapCX/CZ
, instead of having the ControlGate absorb the outer control layer like everywhere else. Beyond the inconsistency, multiple layers of control are futzy to work with, awkward to understand, and harder to decompose. (Hence theisinstance(subgate, CZ)
inControlledGate._decompose_
).Making this consistent seems worthwhile, and given the behavior of most cases, it seems like this was the original goal, but it just got lost in the special nontrivial control values cases.
I'm pretty sure this is just modifying the
CX/CZPowGate.controlled
methods and adding unit tests, so marking as good first issue.The text was updated successfully, but these errors were encountered: