Measureable channels and mixtures #4194
Conversation
|
Pinging @viathor for review. This is somewhat related to the recent |
|
Discussed in Cirq internal sync: needs documentation to make this feature visible to users. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
95-martin-orion
force-pushed
the
cirq-custom-channel
branch
from
2c0a37d to
e66320d
Compare
|
Used |
|
It's unclear to me what's causing the |
|
Pinging @viathor for review. Whatever fix is needed for the failing test shouldn't affect the overall shape of this PR, so you can safely ignore it - unless you happen to know a fix, in which case please don't ignore it 😛 |
|
Re-requesting review - the failing check has been resolved now (seems to have been a notebook parsing issue) |
There was a problem hiding this comment.
These look good up to some minor tweaks in the code. Looking through #2771 it feels like we should definitely have a flag when these are built like: validate=True which indicates whether or not any validation steps should be done (linalg.is_unitary for matrixmixture and linalg.is_cptp for krauschannel). Given how short a fix for #2771 is, would you mind adding something like cirq.linalg.is_cptp ?
|
Addressed review comments and updated the description: #3241 is not fully fixed by this change, as it does not include support for qudits or non-square operators. Qudit support should be fairly straightforward, but non-square operators will require deeper changes to Cirq. |
| raise ValueError('KrausChannel must have at least one operation.') | ||
| num_qubits = np.log2(kraus_ops[0].size) / 2 | ||
| if not num_qubits.is_integer(): | ||
| raise ValueError( |
There was a problem hiding this comment.
Should probably also check that kraus_ops[0].shape[0] == kraus_ops[0].shape[1].
There was a problem hiding this comment.
Done. Also updated a few adjacent checks to use shape instead of size.
| return NotImplemented | ||
| if self._key != other._key: | ||
| return False | ||
| return np.allclose(self._kraus_ops, other._kraus_ops) |
There was a problem hiding this comment.
Kraus representation is not unique, so when measurement key is unspecified this check is insufficient. Instead, we should compare, e.g. the Choi representations which determine the channel uniquely. OTOH, if the measurement key is specified, then different sets of Kraus operators that give rise to the same Choi will generally describe different channels and then the current logic almost applies. I say "almost" since there is the caveat that one could apply any complex phase factor to the Kraus operators and get equivalent KrausChannel.
I think we should do something like this:
if self._key is None and other._key is None:
return np.allclose(cirq.kraus_to_choi(self._kraus_ops), cirq.kraus_to_choi(other._kraus_ops))
return all(cirq.equal_up_to_global_phase(k1, k2) for k1, k2 in zip(self._kraus_ops, other._kraus_ops))This assumes that if measurement key is specified then KrausChannel instances that differ in the order of otherwise equivalent Kraus operators are to be considered unequal. This makes sense since they generate different measurement outcomes.
It'd be good to add unit tests for both cases above. You can use for example these two sets of Kraus operators for the phase damping channel:
- I/2 and Z/2.
- [[1, 0], [0, 0]] and [[0, 0], [0, 1]].
There was a problem hiding this comment.
This is a reasonable operation to have, but I don't think it should be part of the __eq__ operator. It's effectively possible to add a key to a channel with the from_channel method, and calling that method with equal sets of parameters should return equal results. Would a separate method (e.g. equivalent_behavior) be acceptable?
In either case, I'll update the current __eq__ method to use equal_up_to_global_phase.
There was a problem hiding this comment.
Yes, defining a separate method is definitely fine (and TODO is also fine), but please add a comment to tell users that KrausChannel instances that describe the same linear map may be unequal if they're specified via different Kraus operators.
If we go this route then we should probably leave this as is and not use equal_up_to_global_phase. Otherwise it feels like a half-baked attempt at "behavioral" equality (global phase freedom is just a special case of unitary freedom in Kraus representation which is given by diagonal unitaries).
There was a problem hiding this comment.
Added a TODO for this and left the np.allclose in __eq__.
| return MixedUnitaryChannel(mixture=list(protocols.mixture(mixture)), key=key) | ||
|
|
||
| def __eq__(self, other) -> bool: | ||
| if not isinstance(other, MixedUnitaryChannel): |
There was a problem hiding this comment.
What should be the result of equality check on a KrausChannel and a MixedUnitaryChannel that define the same underlying quantum channel?
There was a problem hiding this comment.
I'd be hesitant to return true for such a comparison. The two may have the same effect on the circuit (and thus these should return true for the equivalent_behavior method described above), but there are minor differences between the objects themselves: KrausChannel.has_mixture() is false even if the channel describes a mixture, and MixedUnitaryChannel has better performance due to not needing to calculate operator probabilities.
There was a problem hiding this comment.
Good point. I like the equivalent_behavior idea.
|
Immediate follow-up items:
|
|
@MichaelBroughton, do you have any further comments? |
MichaelBroughton
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
As requested in quantumlib#4194. Can be used for quantumlib#2271. This predicate is meant to be invoked when constructing a channel to verify that the provided Kraus operators actually describe a valid quantum channel. Recommendations for cleaner `is_cptp` behavior or additional test cases are welcome.
_Partially_ addresses quantumlib#3241; qudit and non-square operator support is not part of this PR. Design is presented in [this RFC](https://tinyurl.com/cirq-custom-channel). `KrausChannel` and `MatrixMixture` both serve two purposes: - Provide a base type for users to create their own noisy channels (and mixtures) without creating a new type - Allow channels (and mixtures) to capture the selected operator index in a measurement result The changes to `act_on_state_vector_args.py` enable the second item; everything else in this PR was already possible in Cirq, but previously required users to define their own classes to make use of it.
As requested in quantumlib#4194. Can be used for quantumlib#2271. This predicate is meant to be invoked when constructing a channel to verify that the provided Kraus operators actually describe a valid quantum channel. Recommendations for cleaner `is_cptp` behavior or additional test cases are welcome.
_Partially_ addresses quantumlib#3241; qudit and non-square operator support is not part of this PR. Design is presented in [this RFC](https://tinyurl.com/cirq-custom-channel). `KrausChannel` and `MatrixMixture` both serve two purposes: - Provide a base type for users to create their own noisy channels (and mixtures) without creating a new type - Allow channels (and mixtures) to capture the selected operator index in a measurement result The changes to `act_on_state_vector_args.py` enable the second item; everything else in this PR was already possible in Cirq, but previously required users to define their own classes to make use of it.
Partially addresses #3241; qudit and non-square operator support is not part of this PR. Design is presented in this RFC.
KrausChannelandMatrixMixtureboth serve two purposes:The changes to
act_on_state_vector_args.pyenable the second item; everything else in this PR was already possible in Cirq, but previously required users to define their own classes to make use of it.