-
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
GridDevice gateset, gate_duration, and compilation_target_gateset support #5315
GridDevice gateset, gate_duration, and compilation_target_gateset support #5315
Conversation
d0e90df
to
5b592d7
Compare
5ca5795
to
1ee848d
Compare
…port * Displays a warning about old Cirq version if DeviceSpecification contains gates not recognized by the client. * Added compilation target gateset example in GridDevice docstring
1ee848d
to
ff8aebc
Compare
Pulled in the latest changes. This is now ready for review! |
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.
Left a first round of comments. Main concern is around using the existing target gatesets as it is based on which 2q gate is present in the proto. We tighten the checks to make sure that the constructed compilation target is a strict subset of the constructed gateset.
cirq_gates = [ops.SYC] | ||
fsim_gates.append(ops.SYC) | ||
elif gate_name == 'sqrt_iswap': | ||
cirq_gates = [cirq.SQRT_ISWAP] |
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.
We should not push cirq.SQRT_ISWAP
to cirq_gates since gates_list.extend(cirq_gates)
below will add it to gates_list
; and we will again add the equivalent gates_list.append(ops.FSimGateFamily(gates_to_accept=fsim_gates))
.
The gate durations should also be specified for the corresponding fsim gate family; since we are accepting all equivalents operations across types; instead of specifying it only for cirq.GateFamily(cirq.SQRT_ISWAP)
which is what is being done right now.
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.
Different FSimGates have different durations today:
Cirq/cirq-google/cirq_google/devices/known_devices.py
Lines 304 to 306 in 9459df7
'fsim_pi_4': 32_000, | |
'inv_fsim_pi_4': 32_000, | |
'syc': 12_000, |
And in general, each gate type in DeviceSpecification
can have a separate duration. IIUC including the gate, while redundant, doesn't change gate validation behavior.
Since gate duration is purely informational, IMO it's OK to not have all the equivalent forms of a gate as part of the key. The alternative solution of having duration under the FSimGateFamily and taking the min/max/some other aggregate of the durations of all the different gates necessarily loses some information.
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.
IIUC including the gate, while redundant, doesn't change gate validation behavior.
The purpose of a gateset is description and validation. Having redundant gates makes description more confusing and verbose, which should be avoided.
Different FSimGates have different durations today:
We can insert a separate fsim gate family instance for each fsim gate type in DeviceSpecification; so we can associate gate duration with the corresponding fsim gate family. i.e.
# gate durations dict should contain this.
{cirq.FSimGateFamily(gates_to_accept=[cirq.SQRT_ISWAP]): 32_000,
cirq.FSimGateFamily(gates_to_accept=[cg.SYC]): 12_000}
# instead of .
{cirq.GateFamily(cirq.SQRT_ISWAP): 32_000,
cirq.GateFamily(cg.SYC): 12_000}
In general, we should be consistent with the gate families that we are using for description and validation, so that inconsistencies like the following cannot occur:
$> sqrt_iswap_gate = cirq.FSimGate(-np.pi/4, 0)
# Returns True because of validation across types by FSimGateFamily.
$> assert sqrt_iswap_gate in sqrt_iswap_metadata.gateset
# Should return True but will return False right now because `sqrt_iswap_gate in cirq.GateFamily(cirq.SQRT_ISWAP)` is False
$> assert any(sqrt_iswap_gate in gf for gf in sqrt_iswap_metadata.gate_durations)
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.
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.
Actually your approach currently fails GridDeviceMetadata
validation because all gate duration keys are expected to be in the gateset. Let's discuss in #5427 since it potentially involves a change in GridDeviceMetadata
. Are you onboard with keeping the gate duration the way it is and leaving a TODO for followup?
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. Will change the gatesets to have a separate FSimGateFamily for each 2q gate instead of a single FSimGateFamily.
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. I considered changing the test to a more black-box approach of checking whether certain gates belong in the gateset rather than asserting via gateset equality, but decided against it because we do want to test the exact gateset elements to verify its string representation.
cirq.PhasedXZGate, | ||
cirq.XPowGate, | ||
cirq.YPowGate, | ||
cirq.ZPowGate, |
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 add a cirq.ZPowGate
gate here given that we have special identifiers for virtual_zpow
and physical_zpow
? Note that if cirq.GateFamily(cirq.ZPowGate)
is present in the gateset, it will accept all (i.e. both tagged and untagged) instances of ZPowGate; in which case the physical_zpow
and virtual_zpow
gate families will be irrelevant.
For example: What happens if the proto specification includes phased_xz
and doesn't contain virtual_zpow
? Should a cirq.Z(q)
be accepted or not? What if it contains phased_xz
and physical_zpow
but doesn't contain virtual_zpow
?
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.
Yeah the acceptance of ZPowGates should definitely be controlled entirely by virtual_zpow
and physical_zpow
. Thanks for the catch!
For the case where the spec includes phased_xz
but not virtual_zpow
, because PhysicalZTag()
on a PhasedXZGate
is ignored, a Z gate specified as a PhasedXZGate
should be accepted and will be applied by the device in some way.
I avoided mentioning the alternative of inserting no_compile tags because transforming a circuit which contains WaitGates is the wrong approach as all delays would be scrambled by the transformation.
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.
Aside from some nits, it is fine with me. I think Tanuj should also approve since he is more familiar with compilation target gatesets.
@@ -77,6 +83,85 @@ def _validate_device_specification(proto: v2.device_pb2.DeviceSpecification) -> | |||
) | |||
|
|||
|
|||
def _build_gateset_and_gate_durations( | |||
proto: v2.device_pb2.DeviceSpecification, | |||
) -> Tuple[cirq.Gateset, Dict[cirq.GateFamily, cirq.Duration]]: |
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 is a private method, but I would still add a docstring since it's a little long.
def _build_compilation_target_gatesets( | ||
gateset: cirq.Gateset, | ||
) -> Sequence[cirq.CompilationTargetGateset]: | ||
"""Detects compilation target gatesets based on what gates are inside the gateset.""" |
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.
So, if we support multiple gatesets, do we have a way to decompose to a combination of CZ and sqrt-iswap for instance? Or do we have to pick one?
I think picking one is probably fine, since most grids will likely only support one type of gate, but we should make this clear in the documentation.
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.
Yep we would have to pick one. If there's a use case to compile to both CZ and sqrt-iswap in the future we could revisit; doing so now for every target would probably create a combinatorial explosion of target gatesets.
On the other hand, if a device supports both CZ and sqrt-iswap, and someone wants to compile a circuit containing sqrt-iswap + some other arbitrary 2q gates using a CZ target gateset, the intended behavior is to leave sqrt-iswap gates untouched and compile other 2q gates to CZ + 1q gates. This isn't supported yet by existing target gatesets, but I'm planning to change that.
Will clarify this behavior in documentation.
cirq_gates = [ops.SYC] | ||
fsim_gates.append(ops.SYC) | ||
elif gate_name == 'sqrt_iswap': | ||
cirq_gates = [cirq.SQRT_ISWAP] |
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.
IIUC including the gate, while redundant, doesn't change gate validation behavior.
The purpose of a gateset is description and validation. Having redundant gates makes description more confusing and verbose, which should be avoided.
Different FSimGates have different durations today:
We can insert a separate fsim gate family instance for each fsim gate type in DeviceSpecification; so we can associate gate duration with the corresponding fsim gate family. i.e.
# gate durations dict should contain this.
{cirq.FSimGateFamily(gates_to_accept=[cirq.SQRT_ISWAP]): 32_000,
cirq.FSimGateFamily(gates_to_accept=[cg.SYC]): 12_000}
# instead of .
{cirq.GateFamily(cirq.SQRT_ISWAP): 32_000,
cirq.GateFamily(cg.SYC): 12_000}
In general, we should be consistent with the gate families that we are using for description and validation, so that inconsistencies like the following cannot occur:
$> sqrt_iswap_gate = cirq.FSimGate(-np.pi/4, 0)
# Returns True because of validation across types by FSimGateFamily.
$> assert sqrt_iswap_gate in sqrt_iswap_metadata.gateset
# Should return True but will return False right now because `sqrt_iswap_gate in cirq.GateFamily(cirq.SQRT_ISWAP)` is False
$> assert any(sqrt_iswap_gate in gf for gf in sqrt_iswap_metadata.gate_durations)
A note about CompilationTargetGatesets: | ||
|
||
A circuit which contains `cirq.WaitGate`s will be dropped if it is transformed using | ||
CompilationTargetGatesets generated by GridDevice. To better control circuit timing, insert | ||
WaitGates after the circuit has been transformed. |
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 think this note should go to the docstring of compilation_target_gatesets
property. This is a specific detail of compilation targets we are using today, and should not be tied to the description of 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.
My understanding is that we want to call out WaitGate
because Google devices all support this gate. Is this concern general enough to all devices to be kept in the compilation_target_gateset
property in cirq.GridDeviceMetadata
? If so, I would suggest to keep it in CompilationTargetGateset
instead, since all CompilationTargetGatesets
would have this issue. If not, I would keep it where it is now.
Also, if we were to keep this comment in cirq-core, are there other identity-like gates that may not be negligible that we should call out?
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 think this is mainly used by the Google devices and we don't need to add it to the CompilationTargetGateset
class.
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.
Sounds good, will keep the docstring here then since GridDevice doesn't have a compilation_target_gatesets
property.
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! Thanks for all the changes!
…port (quantumlib#5315) * GridDevice gateset, gate_duration, and compilation_target_gateset support * Displays a warning about old Cirq version if DeviceSpecification contains gates not recognized by the client. * Added compilation target gateset example in GridDevice docstring * Addresses Tanuj's comments * Added GridDevice docstring comment about WaitGates. I avoided mentioning the alternative of inserting no_compile tags because transforming a circuit which contains WaitGates is the wrong approach as all delays would be scrambled by the transformation. * Set gate_duration to None if the spec doesn't have duration info * Addressed Doug's comments * Addressed Tanuj's 2nd round of comments * Break FSimGateFamily into one for each 2q gate type
…port (quantumlib#5315) * GridDevice gateset, gate_duration, and compilation_target_gateset support * Displays a warning about old Cirq version if DeviceSpecification contains gates not recognized by the client. * Added compilation target gateset example in GridDevice docstring * Addresses Tanuj's comments * Added GridDevice docstring comment about WaitGates. I avoided mentioning the alternative of inserting no_compile tags because transforming a circuit which contains WaitGates is the wrong approach as all delays would be scrambled by the transformation. * Set gate_duration to None if the spec doesn't have duration info * Addressed Doug's comments * Addressed Tanuj's 2nd round of comments * Break FSimGateFamily into one for each 2q gate type
…port (quantumlib#5315) * GridDevice gateset, gate_duration, and compilation_target_gateset support * Displays a warning about old Cirq version if DeviceSpecification contains gates not recognized by the client. * Added compilation target gateset example in GridDevice docstring * Addresses Tanuj's comments * Added GridDevice docstring comment about WaitGates. I avoided mentioning the alternative of inserting no_compile tags because transforming a circuit which contains WaitGates is the wrong approach as all delays would be scrambled by the transformation. * Set gate_duration to None if the spec doesn't have duration info * Addressed Doug's comments * Addressed Tanuj's 2nd round of comments * Break FSimGateFamily into one for each 2q gate type
Part of #5050
Adds the remaining major components to GridDevice.
@dstrain115 @MichaelBroughton @tanujkhattar