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

Order of keys shouldn't matter when comparing cirq.google.KeyValueExecutableSpec #5073

Merged
merged 7 commits into from
Mar 17, 2022

Conversation

tonybruguier
Copy link
Contributor

For issue #4734. It was discussed there that the order of the keys should not matter.

@CirqBot CirqBot added the size: S 10< lines changed <50 label Mar 13, 2022
@tonybruguier tonybruguier marked this pull request as ready for review March 13, 2022 19:57
@tonybruguier tonybruguier requested review from wcourtney, a team, vtomole, cduck and verult as code owners March 13, 2022 19:57
@tonybruguier tonybruguier requested a review from mpharrigan March 13, 2022 19:57
@tonybruguier
Copy link
Contributor Author

Thanks, @mpharrigan and PTAL when you have the time? It's not urgent on my side.

@mpharrigan
Copy link
Collaborator

Can you add a section to the docstring about the new equality semantics of KeyValueSpec since we're overwriting the auto-generated dataclass equality method? And add a test for the new equality semantics that constructs the kvspec using tuples and the ordinary constructor?

@tonybruguier
Copy link
Contributor Author

Like so?

@tonybruguier
Copy link
Contributor Author

I think this could be ready to merge. But happy to work more on it if there are further comments, of course.

@mpharrigan mpharrigan added area/workflow automerge Tells CirqBot to sync and merge this PR. (If it's running.) labels Mar 17, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 17, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Mar 17, 2022

Automerge cancelled: A required status check is not present.

Missing statuses: ['Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)']

@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 Mar 17, 2022
@mpharrigan mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 17, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 17, 2022
@CirqBot CirqBot merged commit 90e70c6 into quantumlib:master Mar 17, 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 Mar 17, 2022
@tonybruguier tonybruguier deleted the cirq_4734_B branch March 17, 2022 20:06
tonybruguier added a commit to tonybruguier/Cirq that referenced this pull request Apr 14, 2022
…cutableSpec (quantumlib#5073)

For issue quantumlib#4734. It was discussed there that the order of the keys should not matter.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…cutableSpec (quantumlib#5073)

For issue quantumlib#4734. It was discussed there that the order of the keys should not matter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/workflow size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants