Skip to content
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.

Sign up for GitHub

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

Temporary GridDevice.qubits property #5593

Merged
merged 4 commits into from
Jun 24, 2022
Merged

Temporary GridDevice.qubits property #5593

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9164316
Temporary GridDevice.qubits property
verult Jun 23, 2022
249d02f
Sort the qubit list; verify against the expected qubit list instead i…
verult Jun 23, 2022
14ab21a
Ignoring mypy error around property not being supported (https://gith…
verult Jun 24, 2022
00860d5
Merge branch 'master' into cg-device-refactor/temporary-grid-device-q…
CirqBot Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cirq-google/cirq_google/devices/grid_device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,15 @@ def metadata(self) -> cirq.GridDeviceMetadata:
"""Get metadata information for the device."""
return self._metadata

# Some user code using SerializableDevices gets the qubit list via `device.qubits`.
# This is a stopgap solution to prevent user breakage with the change to GridDevice.
@property # type: ignore
@cirq._compat.deprecated(
deadline='v0.16', fix='Change `device.qubits` to `device.metadata.qubit_set`.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like in most cases the quote-unquote "correct" migration would be to change the entire code-base to expect DeviceMetadata objects at API boundaries instead of Device. Unfortunately, this is a rather large and disruptive change that would have to be done throughout a codebase. What do you think @MichaelBroughton ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky one. The way I see it is any cirq-core features we make ought to make use of cirq.Device or a subclass of cirq.Device that is inside of cirq-core. That way any reasonable vendor device could also be used with those features. Specific Vendors who implement their own device related functionality can then have their APIs expect cirq_<vendor>.VendorDevice (which is kind of what we do now with cirq_google.GridDevice and some of our placement routines like anneal.py). I actually think passing around the devices themselves with their type distinctions is the right call vs the metadata objects which may or may not adhere to the same type distinction as devices. Plus it's not too hard to make a library method do a quick check like:

if device.metadata:
  _do_some_functionality_based_on_metadata_

_do_other_stuff_.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine most of the code will look like

if device.metadata is None:
  raise ValueError("need qubits")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to consider making it non-Optional? My take is a Device without qubit information probably can't do much

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelBroughton

Copy link
Collaborator Author

@verult verult Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted with @MichaelBroughton offline - not all devices have a well-defined qubit set and connectivity graph so the consensus during the design phase was to leave it optional.

)
def qubits(self) -> List[cirq.Qid]:
return sorted(self._metadata.qubit_set)

def validate_operation(self, operation: cirq.Operation) -> None:
"""Raises an exception if an operation is not valid.

Expand Down
8 changes: 8 additions & 0 deletions cirq-google/cirq_google/devices/grid_device_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,3 +510,11 @@ def test_to_proto_empty():
assert len(device.metadata.qubit_pairs) == 0
assert device.metadata.gateset == cirq.Gateset()
assert device.metadata.gate_durations is None


def test_grid_device_qubits():
device_info, spec = _create_device_spec_with_horizontal_couplings()
device = cirq_google.GridDevice.from_proto(spec)

with cirq.testing.assert_deprecated('device.qubits', deadline='v0.16'):
assert device.qubits == device_info.grid_qubits