Skip to content

Allow initialization of MeasurementKeys with empty strings #4445

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

Merged
merged 5 commits into from
Aug 21, 2021

Conversation

dstrain115
Copy link
Collaborator

  • An empty string is still a string and is used occasionally as a
    placeholder in user code. We should support creation of measurement
    keys with empty strings.

- An empty string is still a string and is used occasionally as a
placeholder in user code.  We should support creation of measurement
keys with empty strings.
@dstrain115 dstrain115 requested review from cduck, vtomole and a team as code owners August 20, 2021 12:20
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Aug 20, 2021
@CirqBot CirqBot added the Size: XS <10 lines changed label Aug 20, 2021
@dstrain115
Copy link
Collaborator Author

Question to cirq-maintainers. Is this sufficient to prevent problems with backward incompatibility or should we roll back instead?

@@ -41,7 +41,7 @@ class MeasurementKey:
path: Tuple[str, ...] = dataclasses.field(default_factory=tuple)

def __post_init__(self):
if not self.name:
if self.name is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

name has annotated type str, not Optional[str]. How can it be None? Just from the code it looks like this should be an isinstance check, but I know you intend None to be some kind of placeholder instead.

@CirqBot CirqBot added the size: S 10< lines changed <50 label Aug 20, 2021
@dstrain115 dstrain115 requested a review from Strilanc August 20, 2021 21:20
@mpharrigan
Copy link
Collaborator

Can you add a reference to the PR that introduced the change so we have it on the record. GitHub will also show this PR to any future viewers of the previous one

@smitsanghavi
Copy link
Collaborator

smitsanghavi commented Aug 20, 2021

The PR introducing this change was intentionally breaking since

  1. There was inconsistency between cirq.measure(q) and cirq.MeasurementGate().on(q) for some qubit q. The former would assign the measurement key q while the latter would have an empty string.
  2. We decided(1, 2) that dynamically remapping empty keys based on what qubits are operated on was not a desirable solution.

So, either we could keep this inconsistency, change cirq.measure to stop assigning the "default" key or disallow empty keys in MeasurementGate. We went with the last option since it seemed that empty keys are not really much useful in real world circuits and simulations. Since our Results are keyed by a measurement key string, it doesn't seem too radical to make the key argument sort-of required when constructing a MeasurementGate.

@dstrain115
Copy link
Collaborator Author

@smitsanghavi At the very least, we have to at least temporarily roll this change back, since a significant amount of internal code uses the empty string as a placeholder for "anonymous" measurements. If we want to re-break this, we need to migrate that code first.

@dstrain115 dstrain115 added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 21, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 21, 2021
@CirqBot CirqBot merged commit 454c1a3 into quantumlib:master Aug 21, 2021
@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 Aug 21, 2021
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…b#4445)

- An empty string is still a string and is used occasionally as a
placeholder in user code.  We should support creation of measurement
keys with empty strings.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
…b#4445)

- An empty string is still a string and is used occasionally as a
placeholder in user code.  We should support creation of measurement
keys with empty strings.
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. size: S 10< lines changed <50 Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants