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

Use MeasurementKey in CircuitOperation #4086

Merged
merged 9 commits into from
May 11, 2021

Conversation

smitsanghavi
Copy link
Collaborator

Phase 2a of #4040 RFC.

  • Added a concept of key path. Similar to file path, denotes the path to this particular key in the circuit and distinguishes from other keys with the same name in other paths.
  • Added a with_key_path protocol and the required magic methods to make it work across Cirq.
  • Used these in CircuitOperation to simplify the logic and remove the need for any string parsing.
  • Since the measurement_keys protocol still speaks str rather than MeasurementKey we need to serialize to and from string at the boundaries of this protocol, leading to some string parsing. These are aims to be removed in future phases by introducing an equivalent protocol API with MeasurementKey representations, rather than string.
  • MeasurementKey.parse_serialized helps in constructing the objects at the above boundaries and is the only way to parse nested key string, directly passing them to the constructor will raise ValueError. This is to avoid users from accidentally using the separator character in their keys, thereby confusing our logic that relies on a specific meaning of that character.
  • Change the character used to separate the crosses of repetition ids (but not nesting) to -. For e.g. subcircuit.repeat('a', 'b').repeat('c', 'd') used to create a non-nested CircuitOperation with repetition_ids=['a:c', 'a:d', 'b:c', 'b:d']. But since we want to reserve the character : for separating different nesting levels, we will now have repetition_ids=['a-c', 'a-d', 'b-c', 'b-d'].
  • Also had to change the JSON representations due to the addition of path and added new test cases there as well.

@smitsanghavi smitsanghavi requested review from cduck, vtomole and a team as code owners May 4, 2021 06:43
@smitsanghavi smitsanghavi requested a review from viathor May 4, 2021 06:43
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label May 4, 2021
@95-martin-orion 95-martin-orion requested review from 95-martin-orion and removed request for viathor May 4, 2021 16:33
Copy link
Collaborator

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

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

A few notes on this but otherwise it looks very clean.

cirq-core/cirq/ops/moment_test.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/measurement_gate_test.py Show resolved Hide resolved
@@ -40,40 +40,18 @@ def default_repetition_ids(repetitions: int) -> Optional[List[str]]:
return None


def cartesian_product_of_string_lists(list1: Optional[List[str]], list2: Optional[List[str]]):
def full_join_string_lists(list1: Optional[List[str]], list2: Optional[List[str]]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a private method (i.e. with leading "_")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The test uses it. Not sure what the python convention for this is. Is it okay for unit tests to depend on private methods?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's just the tests using it, I'd prefer making it private. We're not particularly strict about the "no private methods in tests" rule in Cirq.

@smitsanghavi smitsanghavi requested a review from balopat May 5, 2021 18:53
@95-martin-orion
Copy link
Collaborator

After converting full_join_string_lists to private, this should be good to go. Holding off on approval to allow for requested review from @balopat - let me know if you'd like to go ahead without that review.

@smitsanghavi
Copy link
Collaborator Author

After converting full_join_string_lists to private, this should be good to go. Holding off on approval to allow for requested review from @balopat - let me know if you'd like to go ahead without that review.

Done.

@balopat seems to be from last week's Cirq sync when I was trying to demonstrate the issue with adding reviewers. More reviews are definitely welcome but I'd be good with merging this as well.

@95-martin-orion 95-martin-orion merged commit b4c445f into quantumlib:master May 11, 2021
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
* Add the concept of measurement key path and use it for nested/repeated CircuitOperations. Also add `with_key_path` protocol.

* Format and docstrings

* json and other fixes

* Change to immutable default for pylint

* Make full_join_string_lists private
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants