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

Only gate operations supported in CirqGateAsBloq #365

Closed
fdmalone opened this issue Sep 22, 2023 · 5 comments
Closed

Only gate operations supported in CirqGateAsBloq #365

fdmalone opened this issue Sep 22, 2023 · 5 comments
Labels
cirq_interop Issues related to Cirq interop

Comments

@fdmalone
Copy link
Collaborator

Sometimes when decomposing a cirq gate as a bloq I get these errors:

    209 for op in circuit.all_operations():
    210     if op.gate is None:
--> 211         raise ValueError(f"Only gate operations are supported, not {op}.")
    213     bloq = CirqGateAsBloq(op.gate)
    214     for q in op.qubits:

ValueError: Only gate operations are supported, not CZ(control[0, 0], control[1, 0]).with_classical_controls(target[0, 0]).

I can get around it by liberal generalizing during bloq_counts but is it expected? Could we just raise a warning instead of and Error?

@tanujkhattar
Copy link
Collaborator

I think the culprit here is ClassicallyControlledOperation, which is used in many of the cirq specific implementations like AND†.

When I was generating the QROM bloq_counts diagram, I added a condition so that we stop decomposing at AND / AND† and consider these as leaf nodes. Ofc that's not a solution and we still need to figure out what's our story for classically controlled operations in Bloqs - cc @mpharrigan xref #214

@mpharrigan mpharrigan changed the title Only gate operations supported Only gate operations supported in CirqGateAsBloq Sep 29, 2023
@mpharrigan
Copy link
Collaborator

Just from the code in the stack trace: Immediately we use op.gate as the thing we want to wrap.

@mpharrigan mpharrigan added the cirq_interop Issues related to Cirq interop label Sep 29, 2023
@mpharrigan
Copy link
Collaborator

mpharrigan commented Oct 25, 2023

I'm going to close this. Anything that is not a GateOperation is out-of-scope for interop and should be re-written.

Specifically for classically-controled operations we need measurement and classical wires: #445

@daxfohl
Copy link

daxfohl commented Feb 2, 2024

I know I desperately wanted to implement it as a gate, but there was something blocking it. I can't remember offhand what the blocker was though. I'll post again if I remember.

I highly recommend, whatever you do for measurements, keep parity between cbits and qubits. Having cbits represented by just a key in a dictionary while qubits had dimensions and other rich type information was a frequent source of problems.

@daxfohl
Copy link

daxfohl commented Feb 2, 2024

Oh right, because a primary use case was to classically control CircuitOperations, so it wouldn't have made sense if classical control was a gate.

I did have a branch where I reimplemented CircuitOperation as a gate. If I'd have known this would be a problem, I'd have pushed harder on that and then making classical control be a gate as well.

quantumlib/Cirq@main...daxfohl:Cirq:circuitgate

Anyway, hopefully this disparity can be avoided in qualtran. Ensure behavior is always in the gate layer; operations should just be (gate, [register]) tuples or an equivalent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cirq_interop Issues related to Cirq interop
Projects
None yet
Development

No branches or pull requests

4 participants