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

Add support for repeated keys in result protos #5907

Merged
merged 6 commits into from
Oct 25, 2022

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Oct 3, 2022

This is a first step toward supporting repeated keys in circuits run through quantum engine. I need to expand the tests to cover cases where instances > 1, but the main logic is ready for review.

@maffoo maffoo requested review from wcourtney, a team, vtomole, cduck and verult as code owners October 3, 2022 20:34
@CirqBot CirqBot added the size: M 50< lines changed <250 label Oct 3, 2022
Copy link
Collaborator

@wcourtney wcourtney left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but I have a few small questions/comments to resolve.

cirq-google/cirq_google/api/v2/results.py Show resolved Hide resolved
cirq-google/cirq_google/api/v2/results.py Show resolved Hide resolved
//
// The bytes in this field are constructed as follows:
//
// 1. The results of the measurements produce a list of bits ordered by
// the round of repetition.
// the round of repetition and instance within a round.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't notice any tests that confirm this behavior. Can you add them or point me to them? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see tests that you can serialize and deserialize with the expected result, but not that the serialization follows the contract listed in this proto.

cirq-google/cirq_google/api/v2/result.proto Show resolved Hide resolved
@maffoo maffoo requested a review from wcourtney October 21, 2022 21:43
Copy link
Collaborator

@wcourtney wcourtney left a comment

Choose a reason for hiding this comment

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

Overall LGTM % some more test comments :-). Approving since I'm out after today and you should have the gist.

//
// The bytes in this field are constructed as follows:
//
// 1. The results of the measurements produce a list of bits ordered by
// the round of repetition.
// the round of repetition and instance within a round.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see tests that you can serialize and deserialize with the expected result, but not that the serialization follows the contract listed in this proto.

assert isinstance(proto, v2.result_pb2.Result)
assert len(proto.sweep_results) == 2
deserialized = v2.results_from_proto(proto, measurements)
assert len(deserialized) == 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer to test behavior independently rather than rolling many separate assertions into one monolithic test.

go/unit-testing-practices#behavior-testing-examples

@maffoo maffoo merged commit ec68a55 into master Oct 25, 2022
@maffoo maffoo deleted the u/maffoo/proto-repeated-keys branch October 25, 2022 15:56
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.

3 participants