GridDeviceMetadata design discussion and nits.

[tanujkhattar]

Description of the issue
I just looked at the newly added

Cirq/cirq-core/cirq/devices/grid_device_metadata.py

Line 34 in 6937e41

class GridDeviceMetadata(device.DeviceMetadata):

and have some feedback / discussion points:

1. s/supported_gates/gateset/g ? Type of supported_gates is not clear from it's name (is it a list of supported gate instances? gate types? gate families?) and we should change it to a more descriptive name like gateset.
2. gate_durations should be a dict from GateFamily to cirq.Duration instead of Gateset to cirq.Duration. A gateset is a collection of gates, each of which can potentially have different durations.
   • On a related note, we should consider adding a metadata, like duration, to the base GateFamily class itself. Maybe as an optional field? Maybe as a metadata that can be queried?
   • Maintaining an explicit Gateset and an explicit Dict['cirq.GateFamily', 'cirq.Duration'] in the GridDeviceMetadata looks like a bad smell. A gateset is a collection of gates, and duration is a property of gates. Why decouple them and store them independently?
3. If the above is done, then GridDeviceMetadata would consist of just qubit_pairs and a gateset, both of which could potentially move into the Device class, avoiding an additional indirection to access qubits on a device -- (cc Matt's comment on Deprecate device.qubit_set in cirq_google. #4940)

Cirq version
0.14dev

cc @MichaelBroughton @verult @dstrain115

[tanujkhattar]

3. is probably not true, because GridDeviceMetadata derives from DeviceMetadata which comprises of the connectivity graph and qubit set, soGridDeviceMetadata needs to exist. But I'm still not sure why def qubit_set(self) -> FrozenSet['cirq.Qid'] needs to exist on DeviceMetadata instead of on the Device itself ?

[MichaelBroughton]

s/supported_gates/gateset/g ? Type of supported_gates is not clear from it's name (is it a list of supported gate instances? gate types? gate families?) and we should change it to a more descriptive name like gateset.

The name of the property on the class is gateset do we just want to change the name of the variable in the constructor ?

gate_durations should be a dict from GateFamily to cirq.Duration instead of Gateset to cirq.Duration. A gateset is a collection of gates, each of which can potentially have different durations.
On a related note, we should consider adding a metadata, like duration, to the base GateFamily class itself. Maybe as an optional field? Maybe as a metadata that can be queried?
Maintaining an explicit Gateset and an explicit Dict['cirq.GateFamily', 'cirq.Duration'] in the GridDeviceMetadata looks like a bad smell. A gateset is a collection of gates, and duration is a property of gates. Why decouple them and store them independently?

I'm fine with changing to Dict['cirq.GateFamily', 'cirq.Duration'], the point of this class is to just get the information to the users in an exploratory fashion, both GateFamily and Gateset will do this.

Maintaining an explicit Gateset and an explicit Dict['cirq.GateFamily', 'cirq.Duration'] in the GridDeviceMetadata looks like a bad smell. A gateset is a collection of gates, and duration is a property of gates. Why decouple them and store them independently?

Durations can't always be well defined by just the gates. In all cases, the duration of the gate can only be known or defined once you have a device that implements that gate. I think it makes complete sense that a gateset/gatefamily to duration mapping happen close to the device and it be clear that the device dictates this information. I don't think we should add metadata about durations to gatesets to turn their job into communicating information about groups of gates and their respective durations were they to have some known timings related to a device.

[verult]

Thanks for the suggestions!

My take:

1. No strong preference especially since the property name is gateset.
2. I also brought up the same point with Mike :) I agree that both forms serve the informational purpose equally since gates with the same duration can be grouped into the same gateset, but IMO a GateFamily:Duration mapping (a) is more intuitive, since a duration is associated with gates rather than gatesets, and grouping GateFamilies into a Gateset based on duration isn't the intended use of Gateset, and (b) makes the implementation of my current Devices refactor design a tad simpler.
3. Agree with Mike that duration is associated with (gate, device) tuple.

[tanujkhattar]

The name of the property on the class is gateset do we just want to change the name of the variable in the constructor ?

+1, let's make the constructor variable name match the property name.

I'm fine with changing to Dict['cirq.GateFamily', 'cirq.Duration'], the point of this class is to just get the information to the users in an exploratory fashion, both GateFamily and Gateset will do this.

On a closer look, I see that using a Gateset as a key allows you to group gates with same duration together into a single key, but I think it's better to be verbose and accept Dict['cirq.GateFamily', 'cirq.Duration'] instead. This has caused confusion more than once (eg: original design doc, cirq_google devices refactor design doc etc.

the duration of the gate can only be known or defined once you have a device that implements that gate.

Sounds good. Still curious if qubit_set needs to exist on DeviceMetadata instead of on the Device itself ?

[MichaelBroughton]

Closing for now. After offline discussions we agreed that in the new roles that Device take on: Devices do validate_*** and metadata stores all the exploratory items of which qubit_set is a member. Gatedurations and rename updates were here: #4963 .