-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use GoogleCZTargetGateset; add device gateset as additional gates in target gatesets #5765
Use GoogleCZTargetGateset; add device gateset as additional gates in target gatesets #5765
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a minor nit once the first 'GoogleCZTargetGateset' PR is in.
COUPLER_PULSE_GATE_FAMILY = cirq.GateFamily(experimental_ops.CouplerPulse) | ||
MEASUREMENT_GATE_FAMILY = cirq.GateFamily(cirq.MeasurementGate) | ||
WAIT_GATE_FAMILY = cirq.GateFamily(cirq.WaitGate) | ||
_SYC_GATE_FAMILY = cirq.GateFamily(ops.SYC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth a line comment to explain that these are needed to specify compilation targets when constructing a GridDevice from device specification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards no because they can be reused for different purposes in the future. Different gate families here are actually used for different things (serializing to DeviceSpecification, specifying target gates of a CompilationTargetGateset, or deserializing to a gateset).
I'll move FSIM gate families into a separate section though so they can be distinguished more easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synced offline. Added comments and moved Z gate families to constants.
@tanujkhattar PTAL whenever you have spare cycles - would be good to have your approval as well. Or we could also merge as is if you feel comfortable |
MEASUREMENT_GATE_FAMILY = cirq.GateFamily(cirq.MeasurementGate) | ||
WAIT_GATE_FAMILY = cirq.GateFamily(cirq.WaitGate) | ||
# Gate family constants used in various parts of GridDevice logic. | ||
_SYC_GATE_FAMILY = cirq.GateFamily(ops.SYC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we have both _SYC_GATE_FAMILY
and _SYC_FSIM_GATE_FAMILY
. Can you explain briefly? Also happy to sync offline in a 1:1
@@ -400,25 +426,25 @@ def _value_equality_values_(self): | |||
def _set_gate_in_gate_spec( | |||
gate_spec: v2.device_pb2.GateSpecification, gate_family: cirq.GateFamily | |||
) -> None: | |||
if gate_family == SYC_GATE_FAMILY: | |||
if gate_family == _SYC_GATE_FAMILY: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your _build_gateset_and_gate_durations
method inserts a _SYC_FSIM_GATE_FAMILY
into your gateset if the gate specification proto contains an entry for syc
; but when converting from a gateset to a gate specification proto; you check whether the gate family in your gateset is _SYC_GATE_FAMILY
(instead of _SYC_FSIM_GATE_FAMILY
) to set the syc
field in the proto.
Is this expected? or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do we have proto -> gateset -> proto roundtrip tests which test this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GridDevice
cannot be serialized back into DeviceSpecification
currently. I plan to address this after 1.0 by adding a to_device_specification()
method which either returns a cached DeviceSpecification
or converts GridDeviceMetadata
, TBD.
create_device_specification_proto()
doesn't serialize a GridDevice
. It accepts just enough device information to populate the DeviceSpecification
proto, which doesn't contains some of the generated information in GridDevice like target gatesets. The goal was to simplify the population of DeviceSpecification
by not having to manually construct the full GridDevice on server side. Because this function doesn't serialize GridDevice
, this doesn't need to be symmetric with logic in from_proto
, and instead should be simpler. This is why the gate families for syc are different. For create_device_specification_proto()
, I used the simplest GateFamily (_SYC_GATE_FAMILY) to represent the syc gate field in DeviceSpecification
proto. For from_proto()
, _SYC_FSIM_GATE_FAMILY
has to be used to accept all gates equivalent to syc.
Will add a comment in create_device_specification_proto()
to clarify that it doesn't serialize GridDevice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you given me an example of a user who would call create_device_specification_proto
and get back the DeviceSpecification
? Who is this user? Where will they obtain the gateset
that this method expects? Can / should they use the device.metadata.gateset
that's already present on a grid device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One reason you want to generate a DeviceSpecification
proto from a GridDevice is server-side, when you are using the GridDevice (which was probably generated with server-side configuration) for client-sent circuits, and you need to generate device specification.
That being said, I am not quite sure I understand the different between create_device_specification_proto()
and to_device_specification()
. Is this something we can post-pone discussion until after 1.0?
I propose doing the minimum we need to get to 1.0, so we can get this PR out (even if it means removing methods we will have to add back later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @maffoo who also had concerns in this area
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mark it somehow to note that it is an experimental API and may change in the future? (e.g. can we make it an underscore-prefix semi-private function?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, my remaining comment is regarding "The current serialization/deserialization API is confusing to enough people that it definitely needs a revisit after 1.0" and I would like to prevent deprecation (or backwards incompatibility) shortly after we declare that we are not going to do that. I know we left room for cirq_google, but I would like to avoid as much churn as possible. If the best we can do is add an "EXPERIMENTAL: May change" disclaimer to the docstring, that will do, but I would prefer preemptive action if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed to _create_device_specification_proto so my concerns are addressed. I've already approved.
0a5e461
to
9c15358
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from my side. We can merge once @dstrain115's comments are resolved.
if all(gate_family in gateset.gates for gate_family in _CZ_TARGET_GATES): | ||
target_gatesets.append( | ||
transformers.GoogleCZTargetGateset( | ||
additional_gates=(list(gateset.gates - set(_CZ_TARGET_GATES))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove extra parens
additional_gates=(list(gateset.gates - set(_CZ_TARGET_GATES))) | |
additional_gates=list(gateset.gates - set(_CZ_TARGET_GATES)) |
target_gatesets.append( | ||
cirq.SqrtIswapTargetGateset(use_sqrt_iswap_inv=cirq.SQRT_ISWAP_INV in gateset) | ||
cirq.SqrtIswapTargetGateset( | ||
additional_gates=(list(gateset.gates - set(_SQRT_ISWAP_TARGET_GATES))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove extra parens
additional_gates=(list(gateset.gates - set(_SQRT_ISWAP_TARGET_GATES))) | |
additional_gates=list(gateset.gates - set(_SQRT_ISWAP_TARGET_GATES)) |
…l' docstring; removed extra parens
Added an underscore and added docstring to say the function is experimental. I think |
…factor/use-google-cz-target-gateset
…target gatesets (quantumlib#5765) * GridDevice target gateset generation logic now matches all required gates for a particular gateset, rather than just the 2q gate. * In target gatesets that support additional_gates, it's set to all gates in a device's gateset other than required gates in the target gateset, so that device gates are not decomposed during circuit transformation. First commit is from quantumlib#5744 @tanujkhattar cc @dstrain115
First commit is from #5744
@tanujkhattar
cc @dstrain115