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.

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

Conversation

verult
Copy link
Collaborator

@verult verult commented Jun 23, 2022

@verult verult requested a review from mpharrigan June 23, 2022 22:52
@verult verult requested review from wcourtney, a team, vtomole and cduck as code owners June 23, 2022 22:52
@CirqBot CirqBot added the size: S 10< lines changed <50 label Jun 23, 2022
# 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.

Copy link
Collaborator Author

@verult verult Jul 7, 2022

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.

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 24, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 24, 2022
@CirqBot CirqBot merged commit d55a99a into quantumlib:master Jun 24, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jun 24, 2022
@verult verult mentioned this pull request Jul 7, 2022
40 tasks
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants