Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

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

Open daxfohl opened this issue 3 years ago • 2 comments

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 avatar Sep 21 '22 00:09 daxfohl

>>> 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!

tanujkhattar avatar Oct 12 '22 17:10 tanujkhattar

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 avatar Oct 12 '22 22:10 daxfohl

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

daxfohl avatar Oct 27 '22 16:10 daxfohl