Add qubit_attributes to GridDeviceMetadata and corresponding proto#8155
Add qubit_attributes to GridDeviceMetadata and corresponding proto#8155BichengYing wants to merge 3 commits into
Conversation
…roto message definition This will allow use to have more room to add custom qubit corresponding information to the external users in the future. This change should be backward and forwar compatible
BichengYing
requested review from
a team,
hoisinberg,
verult and
wcourtney
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8155 +/- ##
=======================================
Coverage 99.60% 99.60%
=======================================
Files 1118 1118
Lines 101048 101145 +97
=======================================
+ Hits 100647 100745 +98
+ Misses 401 400 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
We should include some example attributes in GridDeviceMetadata.json and GridDeviceMetadata.repr, as well as their initial form to verify legacy json and repr data.
Also it might be better to use a mapping instead of repeated message to express the attributes in a proto.
Unless strongly justified, it should be better to disallow attributes (i.e., fail proto validation) for unknown qubits in GridDevice.
Otherwise LGTM in general.
| 'all_qubits': sorted(self.qubit_set), | ||
| 'compilation_target_gatesets': list(self._compilation_target_gatesets), | ||
| 'qubit_attributes': sorted( | ||
| [(q, sorted(attrs.items())) for q, attrs in self._qubit_attributes.items()], |
There was a problem hiding this comment.
Consider making the qubit_attributes item optional only when set. That way it might be possible to leave GridDevice.json and GridDevice.repr unchanged.
Nit - no need for throw-away list (same on line 195):
| [(q, sorted(attrs.items())) for q, attrs in self._qubit_attributes.items()], | |
| ((q, sorted(attrs.items())) for q, attrs in self._qubit_attributes.items()), |
| dict(gate_durations) if gate_durations is not None else None, | ||
| all_qubits, | ||
| compilation_target_gatesets, | ||
| dict(qubit_attributes) if qubit_attributes is not None else None, |
There was a problem hiding this comment.
qubit_attributes is already copied in __init__, just pass it as is:
| dict(qubit_attributes) if qubit_attributes is not None else None, | |
| qubit_attributes, |
| gate_durations: Mapping[cirq.GateFamily, cirq.Duration] | None = None, | ||
| all_qubits: Iterable[cirq.GridQubit] | None = None, | ||
| compilation_target_gatesets: Iterable[cirq.CompilationTargetGateset] = (), | ||
| qubit_attributes: Mapping[cirq.GridQubit, Mapping[str, Any]] | None = None, |
There was a problem hiding this comment.
Can you provide typing consistent with the QubitAttributeValue proto instead of Any?
This could be done with a TypeVar, for example,
TQubitAttributeValue = TypeVar("TQubitAttributeValue", bound=bool | int | float | str)| # test JSON serialization | ||
| rep_str = cirq.to_json(metadata) | ||
| assert metadata == cirq.read_json(json_text=rep_str) |
There was a problem hiding this comment.
Move this to test_griddevice_json_load above.
| # test repr | ||
| cirq.testing.assert_equivalent_repr(metadata) |
There was a problem hiding this comment.
Move to test_repr above.
| self._metadata = metadata | ||
|
|
||
| @property | ||
| def qubit_attributes(self) -> Mapping[cirq.GridQubit, Mapping[str, Any]]: |
There was a problem hiding this comment.
Please reuse the TQubitAttributeValue type annotation suggested above.
| val_proto = entry.value | ||
| which_val = val_proto.WhichOneof("val") | ||
| if which_val == "bool_value": | ||
| attrs[entry.name] = val_proto.bool_value | ||
| elif which_val == "int_value": | ||
| attrs[entry.name] = val_proto.int_value | ||
| elif which_val == "double_value": | ||
| attrs[entry.name] = val_proto.double_value | ||
| elif which_val == "string_value": |
There was a problem hiding this comment.
(i) Please move this conversion to a local helper function.
(ii) also, please rewrite as a match-case statement like here
| string developer_recommendations = 4; | ||
|
|
||
| // Qubit attributes for the device. | ||
| repeated QubitAttributes qubit_attributes = 6; |
There was a problem hiding this comment.
Can this be a map<string, QubitAttributes> instead? That way there could be no duplicate entries for some qubit.
| string qubit = 1; | ||
| repeated QubitAttributeEntry attributes = 2; |
There was a problem hiding this comment.
Similar as above, this could just have one field of map<string, QubitAttributeValue> attributes which would prevent duplicate specification of some attribute.
| # Add attributes for an unlisted qubit | ||
| qa = spec.qubit_attributes.add() | ||
| qa.qubit = "10_10" # Not in valid_qubits |
There was a problem hiding this comment.
Is this behavior required?
On a first look the qubit_attributes keys should be consistent with the qubits present in the device, so this should raise an error in _validate_device_specification.
If unsure, I'd suggest to leave out this test.
2 participants
This will allow us to have more room to add custom qubit corresponding information to the external users in the future. This change should be backward and forward compatible