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

GridDeviceMetadata: allow qubit pairs in both directions #5241

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

verult
Copy link
Collaborator

@verult verult commented Apr 9, 2022

Fixes #5169

@MichaelBroughton do you think this API makes sense?

@verult verult requested review from a team, vtomole and cduck as code owners April 9, 2022 02:06
@CirqBot CirqBot added the size: M 50< lines changed <250 label Apr 9, 2022
@verult verult force-pushed the griddevicemetadata-qubitpairs branch from 1b5f2b5 to 176a36d Compare April 9, 2022 07:21
@verult verult marked this pull request as draft April 9, 2022 07:22
@verult verult force-pushed the griddevicemetadata-qubitpairs branch from 176a36d to 7f6375d Compare April 9, 2022 07:24
@MichaelBroughton
Copy link
Collaborator

This looks good. We are already sorting QIDs here as per @dstrain115 's comment in the issue (so things like serialization are not order dependent). Since this change is happening in the subtype and not the parent type (DeviceMetadata), how would we feel about just adding the (r, l) pairs in the tuple to go along with the exsiting (l, r) pairs in sorted order ? We wouldn't have to introduce a new type and it would keep things a little simpler.

@verult verult force-pushed the griddevicemetadata-qubitpairs branch 3 times, most recently from df7c037 to 22c3fa1 Compare April 25, 2022 23:58
@verult verult marked this pull request as ready for review April 25, 2022 23:58
@verult
Copy link
Collaborator Author

verult commented Apr 26, 2022

Discussed offline. Will instead use a FrozenSet[Frozenset[cirq.Qid]] to represent a collection of qubit pairs, similar to the deprecated SymmetricalQidPair but keeping it simple by using a frozenset.

The option of keeping both (l, r) and (r,l) in the set has the downside that len(qubit_pairs) is 2x the actual size.

@verult verult marked this pull request as draft April 26, 2022 00:58
@verult verult force-pushed the griddevicemetadata-qubitpairs branch from 22c3fa1 to e49b319 Compare April 26, 2022 21:29
@verult verult marked this pull request as ready for review April 26, 2022 21:46
@verult verult requested a review from wcourtney as a code owner April 26, 2022 21:46
@verult
Copy link
Collaborator Author

verult commented Apr 26, 2022

Pushed the latest changes. I opted to keep the qubit_pairs in the constructor as a Frozenset[Tuple[Qid, Qid]] to enforce the requirement that the pair must have exactly 2 elements, and since ordering doesn't matter in the constructor, using the pair as a frozenset isn't necessary.

My preference is to reuse the SymmetricalQidPairs which is being deprecated and removed in #5290, which:

  • Requires that the pair has exactly 2 elements, unlike a bare frozenset
  • Better describes the intention of the data structure for the pair.
    In this case we could use SymmetricalQidPairs in the constructor as well.

Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelBroughton MichaelBroughton self-assigned this Apr 26, 2022
@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 26, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 26, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Apr 26, 2022

Automerge cancelled: Pull Request is not mergeable.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 26, 2022
@MichaelBroughton MichaelBroughton added automerge Tells CirqBot to sync and merge this PR. (If it's running.) and removed front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Apr 26, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 26, 2022
@CirqBot CirqBot merged commit 3c6e641 into quantumlib:master Apr 26, 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 Apr 26, 2022
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: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GridDeviceMetadata: qubit ordering within a pair
3 participants