Add a MeasurementKey class and make it the internal representation in MeasurementGate
#4039
Conversation
|
@95-martin-orion (since I can't manually add reviewers) |
95-martin-orion
requested review from
95-martin-orion
and removed request for
viathor
| def key(self, key_str: str): | ||
| # TODO: Allow nested keys only when created via some special methods. Planned in Phase 2a | ||
| # of https://tinyurl.com/structured-measurement-keys#heading=h.zafcj653k11m | ||
| self.mkey = value.MeasurementKey(key_str, allow_nested_key=True) |
There was a problem hiding this comment.
would it make more sense to set allow_nested_key to False until the full logic is in place? Otherwise we could break stuff in between.
There was a problem hiding this comment.
Actually, I'm setting it to True to ensure nothing breaks. Currently, a decomposed CircuitOperation will has these nested keys inside regular MeasurementGates (nested keys are just keys with a : inside).
Until Phase 2a of the RFC, the MeasurementKey and MeasurementGate classes will not know if their keys are being remapped in a way that they need to allow nested keys. So setting a blanket allow to ensure nothing triggers the ValueError (except if someone creates a nested MeasurementKey directly).
| def __str__(self): | ||
| return self.name | ||
|
|
||
| def __hash__(self): |
There was a problem hiding this comment.
Ignore this comment if we really want to exclude allow_nested_key from the hash.
This and the _hash field are unnecessary: since both eq and frozen are true for this class, a hash function will be auto-generated: dataclass docs.
There was a problem hiding this comment.
Yeah, I defined them specifically to exclude allow_nested_key. I feel it should just be a InitVar but had to keep it around for serialization. More on it in the comment below.
|
|
||
| def __hash__(self): | ||
| if self._hash is None: | ||
| object.__setattr__(self, '_hash', hash(self.name)) |
There was a problem hiding this comment.
Nit: include allow_nested_key in the hash?
There was a problem hiding this comment.
I don't think we should. WDYT?
| mkey = cirq.MeasurementKey('') | ||
| assert mkey.name == '' |
There was a problem hiding this comment.
Now that we have a class, we might as well prevent empty keys - they can only generate confusion.
There was a problem hiding this comment.
Agreed. A bunch of tests failed since sometimes we don't bother naming the measurements in tests. I didn't want to clutter this PR with all that changes so will send that in a subsequent PR as soon as this is in.
There was a problem hiding this comment.
If you don't name the measurement doesn't it give it an automatic name of str(qubits)?
There was a problem hiding this comment.
cirq.measure does. But cirq.MeasurementGate, being a gate, does not know what qubits it's going to be operating on. So the tests which directly use the gate with on would fail and need explicit keys.
As part of special treatment for qubits (phase 2b of the RFC), MeasurementGate.on would be overridden to pass the qubit info to MeasurementKey which would do remapping and maybe create the default qubit-based key, if needed.
There was a problem hiding this comment.
Thanks for the clarification!
There was a problem hiding this comment.
Sounds good to me - we can handle extra validation in subsequent PRs.
| mkey = cirq.MeasurementKey('') | ||
| assert mkey.name == '' |
There was a problem hiding this comment.
Sounds good to me - we can handle extra validation in subsequent PRs.
95-martin-orion
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
| @@ -0,0 +1 @@ | |||
| [cirq.MeasurementKey('key'), cirq.MeasurementKey('nested:key')] | |||
There was a problem hiding this comment.
Was this generated by actually calling repr(g) for some object g ?
The _repr_ above returns f'cirq.MeasurementKey(name={self.name})', which seems wrong. See #4440
…in `MeasurementGate` (quantumlib#4039) Phase 1 of the RFC at https://tinyurl.com/structured-measurement-keys#heading=h.7xjjz7p4hj9y Part of quantumlib#4040
Phase 1 of the RFC at https://tinyurl.com/structured-measurement-keys#heading=h.7xjjz7p4hj9y
Part of #4040