Skip to content
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

2 Qubit ControlledOperations should be supported in convert_to_sycamore_gates #4152

Closed
tanujkhattar opened this issue Jun 1, 2021 · 5 comments · Fixed by #4459
Closed

2 Qubit ControlledOperations should be supported in convert_to_sycamore_gates #4152

tanujkhattar opened this issue Jun 1, 2021 · 5 comments · Fixed by #4459
Assignees
Labels
area/gate-compilation kind/bug-report Something doesn't seem to work. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add

Comments

@tanujkhattar
Copy link
Collaborator

tanujkhattar commented Jun 1, 2021

Description of the Issue

We check isinstance(op, GateOperation) while verifying that the given operation can be decomposed using analytical / tabulation methods. This leads to the problem that we don't recognize two qubit controlled operations as known gates and hence don't support them.

elif len(op.qubits) == 2 and isinstance(op, ops.GateOperation):

For example:

num_qubits = 3
qubits = cirq.GridQubit.rect(1, num_qubits, 4, 4)
circuit = cirq.Circuit(cirq.QuantumFourierTransformGate(num_qubits)(*qubits))
cirq.Circuit(cirq.google.ConvertToSycamoreGates().convert(circuit), device = cirq.google.Sycamore)

The above code fails with the following error:

TypeError: Don't know how to work with cirq.S(cirq.GridQubit(4, 4)).controlled_by(cirq.GridQubit(4, 5)). It isn't a native xmon operation, a 1 or 2 qubit gate with a known unitary, or composite.

The reason it fails is because ControlledOperations don't satisfy the isinstance(gate, GateOperation) constraint.

op = cirq.S(cirq.GridQubit(4, 4)).controlled_by(cirq.GridQubit(4, 5))
print(type(op), isinstance(op, cirq.GateOperation))

gives

<class 'cirq.ops.controlled_operation.ControlledOperation'> False

However, the gate is actually just a two qubit controlled gate and should be decomposable into the native gateset via known_two_q_operations_to_sycamore_operations.

op = cirq.S(cirq.GridQubit(4, 4)).controlled_by(cirq.GridQubit(4, 5))
print([*known_two_q_operations_to_sycamore_operations(op.qubits[0], op.qubits[1], op, None)])

gives the following decomposition

[cirq.PhasedXZGate(axis_phase_exponent=-1.1102230246251565e-16, x_exponent=0.7239292897100449, z_exponent=0.08333333333333348).on(cirq.GridQubit(4, 5)), cirq.PhasedXZGate(axis_phase_exponent=0.5416666666666665, x_exponent=0.0, z_exponent=0.916666666666667).on(cirq.GridQubit(4, 4)), cirq_google.SYC.on(cirq.GridQubit(4, 5), cirq.GridQubit(4, 4)), cirq.Rx(rads=0.5922772837339743).on(cirq.GridQubit(4, 4)), cirq_google.SYC.on(cirq.GridQubit(4, 5), cirq.GridQubit(4, 4)), cirq.PhasedXZGate(axis_phase_exponent=-0.08333333333333315, x_exponent=0.7239292897100451, z_exponent=-0.916666666666667).on(cirq.GridQubit(4, 5)), cirq.PhasedXZGate(axis_phase_exponent=0.22741638234956674, x_exponent=2.220446049250313e-16, z_exponent=0.25).on(cirq.GridQubit(4, 4)), cirq.Rz(rads=0.7853981633974483).on(cirq.GridQubit(4, 5)), cirq.Rz(rads=0.7853981633974483).on(cirq.GridQubit(4, 4))]

Proposed Solution

elif len(op.qubits) == 2 and isinstance(op, ops.GateOperation):

should be modified to also recognize two qubit ControlledOperations.

Cirq version
0.11.0.dev

Part of #3242

@tanujkhattar tanujkhattar added the kind/bug-report Something doesn't seem to work. label Jun 1, 2021
@smitsanghavi
Copy link
Collaborator

I'm assuming the GateOperation check is supposed to filter out Operations that don't have gates.

We have had these discussions about Operation hierarchy and how the type checks are used as proxies for whether there is an underlying Gate or not. I think it's useful to have a has_gate protocol that actually tries to get the underlying gate and performs not-None, etc checks. This would be more pythonic than type checking IMO.

@maffoo
Copy link
Contributor

maffoo commented Jun 1, 2021

op.gate is not None seems about as simple as cirq.has_gate(op), and I think that's basically all the has_gate method would do anyway. Certainly the isinstance(op, cirq.GateOperation) checks are too restrictive in this case.

@smitsanghavi
Copy link
Collaborator

The protocol will also be doing a getattr check for gate first. op.gate is not None assumes that op is some Operation type which might not be the case for all instances of these checks in Cirq.

@viathor viathor added area/gate-compilation triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Jun 14, 2021
@tanujkhattar
Copy link
Collaborator Author

Upd: Similarly, _is_native_xmon_op checks for isinstance(op, cirq.GateOperation) and hence does not recognize gates like CS(0, 1) to be native gates which leads to much worse (decomposed) target circuit lengths. xref #4172

@vtomole
Copy link
Collaborator

vtomole commented Sep 29, 2021

Discussed in Cirq sync: Recent work on the circuit transformer APIs should fix this.

@Strilanc Strilanc added triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Sep 29, 2021
CirqBot pushed a commit that referenced this issue Oct 12, 2021
…ers to support other operation types. (#4459)

Proliferation of `isinstance(op, GateOperation)` checks results in many inconsistencies due to different available operation types like `ControlledOperations` and `TaggedOperations`. This PR fixes #4152 and is a first step towards fixing #3556 

Note that `TaggedOperations` which were earlier ignored by the optimizers would now be considered, and hence this is potentially a breaking change if people were implicitly relying on TaggedOperations not getting compiled by the optimizers. Since the optimizer doesn't document / test this behavior, I consider it to be a bug rather than a feature and an explicit `NoCompile` tag should be implemented as part of #4253 


This PR is blocked on submitting #4167 (tests will stop failing once the PR is submitted and this rebased). 

Update: This is now ready for review.
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
…ers to support other operation types. (quantumlib#4459)

Proliferation of `isinstance(op, GateOperation)` checks results in many inconsistencies due to different available operation types like `ControlledOperations` and `TaggedOperations`. This PR fixes quantumlib#4152 and is a first step towards fixing quantumlib#3556 

Note that `TaggedOperations` which were earlier ignored by the optimizers would now be considered, and hence this is potentially a breaking change if people were implicitly relying on TaggedOperations not getting compiled by the optimizers. Since the optimizer doesn't document / test this behavior, I consider it to be a bug rather than a feature and an explicit `NoCompile` tag should be implemented as part of quantumlib#4253 


This PR is blocked on submitting quantumlib#4167 (tests will stop failing once the PR is submitted and this rebased). 

Update: This is now ready for review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gate-compilation kind/bug-report Something doesn't seem to work. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add
Projects
None yet
6 participants