Skip to content

X.controlled(cirq.SumOfProducts([[1]])) does not optimize to CX #5883

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

Closed
daxfohl opened this issue Sep 21, 2022 · 3 comments · Fixed by #5873
Closed

X.controlled(cirq.SumOfProducts([[1]])) does not optimize to CX #5883

daxfohl opened this issue Sep 21, 2022 · 3 comments · Fixed by #5873
Labels
area/gates kind/bug-report Something doesn't seem to work. status/needs-agreed-design We want to do this, but it needs an agreed upon design before implementation triage/needs-more-evidence [Feature requests] Seems plausible, but maintainers are not convinced about the use cases yet

Comments

@daxfohl
Copy link
Collaborator

daxfohl commented Sep 21, 2022

X.controlled simplifies to CX when possible (and Z to CZ). However this does not work when the control_values is represented as a SumOfProducts, requiring workarounds by any code expecting such optimizations.

A fix for this is provided in #5873.

@daxfohl daxfohl added the kind/bug-report Something doesn't seem to work. label Sep 21, 2022
@viathor viathor added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque area/gates labels Oct 4, 2022
@tanujkhattar tanujkhattar added triage/needs-more-evidence [Feature requests] Seems plausible, but maintainers are not convinced about the use cases yet status/needs-agreed-design We want to do this, but it needs an agreed upon design before implementation and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Oct 12, 2022
@tanujkhattar
Copy link
Collaborator

>>> gate1 = X.controlled(cirq.SumOfProducts([[1]]))
>>> isinstance(gate1, cirq.ControlledGate) # Right now
>>> isinstance(gate1, cirq.CXPowGate) # After the change

This is probably backwards incompatible. Potential considerations:

  • LSP is not strictly satisfied because cirq.CXPowGate is not a subclass of cirq.ControlledGate
  • However, the behavior of the returned object should be "similar" -- is that true? is that enough?
  • Also, expected return type of X.controlled is a cirq.Gate -- so technically it's not a breaking change because the return type is still a Gate. And therefore LSP is also satisfied.

One open question:

  • Is there any case where controlled gate returned right now would have a significantly different behavior than CXPowGate which would end up breaking existing code. If not, then let's do it!

@daxfohl
Copy link
Collaborator Author

daxfohl commented Oct 12, 2022

decompose on X.controlled(cirq.SumOfProducts([[1]])) currently throws TypeError: object of type '<class 'cirq.ops.controlled_gate.ControlledGate'>' does have a _decompose_ method, but it returned NotImplemented or None. So going ahead with this strictly benefits that situation.

Also note that all other cases for X.controlled(...) already return CX when possible. e.g. X.controlled(cirq.ProductOfSums([[1]]) returns CX. Just SumOfProducts([[1]]) doesn't, even though it represents the same control value. So fixing this improves consistency.

https://github.com/quantumlib/Cirq/blob/master/cirq-core/cirq/ops/common_gates_test.py#L125-L128

@daxfohl
Copy link
Collaborator Author

daxfohl commented Oct 27, 2022

@tanujkhattar Any further thoughts on this? The linked PR is ready to go. I added additional details in the comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gates kind/bug-report Something doesn't seem to work. status/needs-agreed-design We want to do this, but it needs an agreed upon design before implementation triage/needs-more-evidence [Feature requests] Seems plausible, but maintainers are not convinced about the use cases yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants