Change GridDeviceMetadata qubit type to GridQubit #5633
Conversation
|
Seems like a good idea to me! |
There was a problem hiding this comment.
do you want to remove the casts that someone linked to in our chat in a recent @MichaelBroughton pr?
|
@mpharrigan Good point, will fix all the casts used in the codebase. |
…SerializableDevice
verult
requested review from
wcourtney,
a team,
vtomole and
cduck
as code owners
|
|
verult
force-pushed
the
griddevicemetadata-gridqid
branch
from
6dc35a7 to
4ff386e
Compare
| Returns: | ||
| Frozenset of qubits on device. | ||
| """ | ||
| return cast(FrozenSet['cirq.GridQubit'], super().qubit_set) |
There was a problem hiding this comment.
wait, why do we need a cast here? Is it because you want to call super() instead of returning frozenset(self.qubits)?
There was a problem hiding this comment.
Right. self.qubits doesn't exist, and I wasn't sure about adding a self._qubits attribute just for this when it's already accessible from the superclass but just with a more general type.
edit: I tried it without the cast and actually mypy didn't complain locally. Will push it and see if it passes presubmits.
There was a problem hiding this comment.
Nope that didn't work, reverting back
| if gate_def.number_of_qubits == 2 | ||
| for pair in gate_def.target_set | ||
| if len(pair) == 2 | ||
| and pair[0] < pair[1] |
There was a problem hiding this comment.
I know this was already here, but does this mean if you order your qubits "backwards" in target_set the metadata will report that there are no pairs?
There was a problem hiding this comment.
Ah good catch. This was added when GridDeviceMetadata required the pair to be ordered but missed this call site when it was changed to be order-agnostic. Will fix separately.
| and isinstance(pair[0], cirq.GridQubit) | ||
| and isinstance(pair[1], cirq.GridQubit) |
There was a problem hiding this comment.
doesn't this obviate the need for the cast? Usually mypy can use if isinstance(...) checks to infer variable types
There was a problem hiding this comment.
I was hoping for that too, but unfortunately mypy complained (that the type is List[Tuple[Qid, Qid]])
| qubit_pairs=cast( | ||
| List[Tuple[cirq.GridQubit, cirq.GridQubit]], | ||
| [ | ||
| (pair[0], pair[1]) |
There was a problem hiding this comment.
put the cast here so you're casting less things? Like it still can infer the List[Tuple[... part; you need to cast the qubits themselves. Although with the isinstance checks I'm sad that that's necessary, see below.
There was a problem hiding this comment.
I tried that at first, but mypy still complained (don't remember the exact error message, though)
| @@ -304,7 +304,7 @@ def place_line(self, device: 'cirq_google.GridDevice', length: int) -> GridQubit | |||
| if not device.metadata.qubit_set: | |||
| return GridQubitLineTuple() | |||
|
|
|||
| start: GridQubit = cast(GridQubit, min(device.metadata.qubit_set)) | |||
verult
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Proposal to change qubit types in `GridDeviceMetadata` to `GridQubit`, per @maffoo 's and @mpharrigan 's suggestion. Looks like `GridQid` is not a superclass of `GridQubit`. Also included a quick docstring fix to disambiguate from `cirq_google.GridDevice`. @MichaelBroughton WDYT?
Proposal to change qubit types in `GridDeviceMetadata` to `GridQubit`, per @maffoo 's and @mpharrigan 's suggestion. Looks like `GridQid` is not a superclass of `GridQubit`. Also included a quick docstring fix to disambiguate from `cirq_google.GridDevice`. @MichaelBroughton WDYT?
Proposal to change qubit types in
GridDeviceMetadatatoGridQubit, per @maffoo 's and @mpharrigan 's suggestion.Looks like
GridQidis not a superclass ofGridQubit.Also included a quick docstring fix to disambiguate from
cirq_google.GridDevice.@MichaelBroughton WDYT?