Add coupler pulse gate #4130
Add coupler pulse gate #4130
Conversation
| serialized_gate_id='pulse', | ||
| args=[ | ||
| op_serializer.SerializingArg( | ||
| serialized_name='coupling_mhz', serialized_type=float, op_getter='coupling_mhz' |
There was a problem hiding this comment.
Internally we tend to use the standard case for units, even when it clashes with case conventions, so this would be coupling_MHz.
There was a problem hiding this comment.
This violates case conventions per Google python style guide, so I don't think it is a good convention to follow.
|
Do we want to add this gate to |
I am not sure we want this as a supported gate of XMonDevice. Sycamores are not XMonDevice objects. |
|
A review of this PR next week would be nice, so we can get this in and proceed. |
🤦 oh of course. However, my main point was that this gate is not included in the duration logic anywhere. |
gate_duration_picos is only useful client-side. It is not used anywhere except to create the device specification in order to be used in cirq client-side (for instance, for noise models or timing diagrams). I don't see a great way to add this to device timing right now, since it has a variable length. We currently don't have a way to set a variable length in the device specification, so we currently leave it blank. WaitGate is also in this class (and the "fsim" serializer, since SYC, identity, and sqrt(iSWAP) have different lengths). We could put in a lambda function somewhere, but then that would break the 'serializable' portion of the serializable device. I'm reluctant to really spend too much time in this, since this is a experimental feature that is interacting with a design that is currently in-flight (Device durations). |
cirq-google/cirq_google/__init__.py
Outdated
| @@ -127,6 +127,10 @@ | |||
| SerializableGateSet, | |||
| ) | |||
|
|
|||
| from cirq_google.experimental.ops import ( | |||
There was a problem hiding this comment.
This essentially makes this feature "public" - how about just keeping it public within cirq_google.experimental? experimental is already imported to the cirq_google namespace.
There was a problem hiding this comment.
Also,cirq_google.experimental is setup in the json testing framework as well.
There was a problem hiding this comment.
That's fine, but it does imply an eventual breakage if we ever move it out of experimental for anyone using it. Given that it is in experimental, I suppose that's okay, but it will inconvenience a handful of researchers.
There was a problem hiding this comment.
Yeah, we can then deprecate the experimental reference and gracefully transition. It might just be dropped eventually as well, and then that will be the deprecation. In the meantime though we don't inconvenience other users of cirq_google by adding it to the main namespace.
| def __repr__(self) -> str: | ||
| return ( | ||
| 'cirq_google.experimental.ops.coupler_pulse.' | ||
| + f'CouplerPulse(hold_time={repr(self.hold_time)}, ' |
There was a problem hiding this comment.
nit: Can use formatting code to get the repr:
| + f'CouplerPulse(hold_time={repr(self.hold_time)}, ' | |
| + f'CouplerPulse(hold_time={self.hold_time!r}, ' |
Same thing applies in a few places below.
There was a problem hiding this comment.
I didn't know you could do that. Cool. Done.
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Adds a shaped trapezoidal pulse gate for experimental gate sets. - Adds `cirq_google.experimental.ops.CouplerPulse` - Adds json serialization - Adds google api serialization
Adds a shaped trapezoidal pulse gate for experimental gate sets.
cirq_google.experimental.ops.CouplerPulse