Strict BloqAsCirqGate and CirqGateAsBloq#1603
Conversation
| cbloq = bb.finalize(ctrl=ctrl, target=target) | ||
| target = bb.add(ZeroState()) | ||
| q = [*ctrl, target] | ||
| c0, c1, target = bb.add(CirqGateAsBloq(and_gate), q=q) |
There was a problem hiding this comment.
This seems like a backwards incompatible change? The CirqGateAsBloq doesn't preserve the signature anymore if the underlying gate is a GateWithRegisters ?
In theory, this should be fine since GateWithRegisters are bloqs so we should use them without wrapping in CirqGateAsBloq, but I'm nervous that this might break stuff in the interop. The tests are passing so that's a good indication, but have you looked into any potential consequence of this?
There was a problem hiding this comment.
One way to be full proof could be to raise an error if a user tries to wrap a GateWithRegisters into a CirqGateAsBloq (or any other object that satisfies isinstance(gate, Bloq)).
I don't like the current behavior where it silently works, but does the wrong thing. It's better to explicitly raise an error if we don't want / expect users to wrap bloq objects in this wrapper
There was a problem hiding this comment.
Yes, I can add some error checking since this changes the behavior of using CirqGateAsBloq with a GateWithRegisters.
It was on purpose to make CirqGateAsBloq only use the cirq.Gate API, which doesn't have a notion of signatures. If we wanted to preserve the "special casing", I would (theoretically) argue for a different adapter: GateWithRegistersAsBloq... but this is redundant :)
There was a problem hiding this comment.
actually, I don't know if an error is appropriate: since GateWithRegisters is a cirq.Gate, you should be able to wrap it -- but it will treat it as a cirq gate. Maybe I'll emit a warning?
There was a problem hiding this comment.
It won't silently fail anyways: if you were wrapping a sided GateWithRegisters with CirqGateAsBloq, the change will change the signature; which will raise an error when you're trying to wire it up.
And of course the fix is simple: remove the adapter! I've added a warning
tanujkhattar
left a comment
tanujkhattar
left a comment
There was a problem hiding this comment.
Main feedback is to explicitly raise an error if CirqGateAsBloq is passed a Bloq object, instead of silently ignoring the LEFT / RIGHT registers and making everything a THRU register.
|
This PR triggers #1336 |
|
I'm going to revert the part of |
|
pytest time back to normal (lower than normal?) |
* strict BloqAsCirqGate and CirqGateAsBloq * fixy * Warn when wrapping a bloq * let cirq handle _unitary_ still
2 participants
We use the adapter pattern for adapting cirq and qualtran (bloq) obejcts, namely
BloqAsCirqGateandCirqGateAsBloq. This PR does not modifyqualtran.GateWithRegisters, which acts simultaneously as a cirq gate and qualtran bloq.BloqAsCirqGateis now only acirq.Gate.This PR removes the
signatureandon_registersmethods fromBloqAsCirqGate(cirq.Gate). These methods are bloq-y and one shouldn't rely on them being available on something that purports to be a cirq gate.BloqAsCirqGatealso learns a new_has_unitary_implementation, which fixes #1570. This fix was revealed when I encountered a related bug while writing this PR: one of the unit tests started taking a long (indefinite) time; which was again traced tocirq.Gate.controlled_bytrying to validate that the gate was unitary.CirqGateAsBloqis now only aBloq.This PR removes the
GateWithRegisters(Bloq, cirq.Gate)functionality from the adapter. Only the bloq API is available onCirqGateAsBloq(so things like_unitary_, diagram info, ... are removed).The functionality for
CirqGateAsBloqis (still) contained inCirqGateAsBloqBasewhich can be used to bootstrap bloq definitions.