Fix #855: don't call cirq.unitary() on non-unitary matrices#857
Fix #855: don't call cirq.unitary() on non-unitary matrices#857
Conversation
In Cirq 1.6.0, the function [`unitary(...)`](https://github.com/quantumlib/Cirq/blob/fe0dc2187ca3269c178526e8ba41083fa1a467c9/cirq-core/cirq/protocols/unitary_protocol.py#L81) in `cirq-core/cirq/protocols/unitary_protocol.py` changed. Whereas previously, if the value passed in was a NumPy array, it the function returned the array directly without doing anything else, `unitary(...)` was changed in [PR the array really _is_ unitary and raise an exception if it isn't. This arguably corrected a fault in `unitary(...)` because with the previous definition, it would return non-unitary results if given a NumPy array that wasn't already unitary. However, in one of the tests in qsim, it had the result of revealing a small bug: the test case was knowingly invoking `unitary(...)` with a non-unitary matrix. This led to exceptions being raised in Cirq 1.6.0 but not in prior versions. The solution in this case turned out to be very simple: the call to `cirq.unitary(...)` on line 1060 was unnecessary because the previous version of `unitary(...)` simply returned it right away. Removing the call kept the previous behavior (which was to use a non-unitary matrix in that situation) and works in both Cirq 1.5.0 and 1.6.0.
github-actions
Bot
added
size: S
|
Note: the docker build failure is due to an unrelated issue. |
|
|
||
| def _mixture_(self): | ||
| return [(prob, cirq.unitary(op)) for prob, op, in self._prob_op_pairs] | ||
| return [(prob, op) for prob, op, in self._prob_op_pairs] |
There was a problem hiding this comment.
Is _mixture a standard protocol for qsim or is this only used for this test? This is changing the return type for this method.
There was a problem hiding this comment.
Oh, I didn't realize op was already a np.array.
There was a problem hiding this comment.
Hmm. It's true that the 2nd element in the tuples in self._prob_op_pairs are actually NoiseStep objects. Lemme see if I can improve this.
There was a problem hiding this comment.
OK, with the way this all routed between different functions, I can't find a way to avoid changing this return type without rewriting test_multi_qubit_noise(). Given that this object class is only used in this file and only for that one function, that seems like it would be poor ROI.
I added a comment explaning what is happening. @dstrain115 can you check that it makes sense?
There was a problem hiding this comment.
Oh, I didn't realize op was already a np.array.
Actuallly, I was wrong about that in my earlier comment (which I deleted too slowly to save you typing that reply -- sorry about that). At this point in the flow of things, it's not an ndarray. The whole flow is a bit more complicated, which hopefully the new comment clarifies.
| def _mixture_(self): | ||
| return [(prob, cirq.unitary(op)) for prob, op, in self._prob_op_pairs] | ||
| # Cirq's mixture() function in mixture_protocol.py returns tuples of | ||
| # the form (probability, unitary peration). It does this by applying |
There was a problem hiding this comment.
peration --> operation
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
LGTM with 1-char typo fixed
|
@mhucka, @dstrain115 - should we remove the is_unitary introduced in quantumlib/Cirq#7419? (or we could replace it with a check for a square matrix) As it is |
By "remove IMHO, removing the check for whether it is unitary would go against the purpose of |
|
I opend quantumlib/Cirq#7572 to follow-up on the issue that |
Fix typo spotted by @pavoljuhas
3 participants
In Cirq 1.6.0, the function
unitary(...)incirq-core/cirq/protocols/unitary_protocol.pychanged. Whereas previously, if the value passed in was a NumPy array, the array was returned without doing anything else,unitary(...)was changed in Cirq PR #7536 to test that the NumPy array really is unitary and raise an exception if it isn't. This arguably corrected a fault inunitary(...)because with the previous definition, it would return non-unitary results if given a NumPy array that wasn't already unitary. However, one of the tests in qsim depended on this flaw: the test case knowingly invokedunitary(...)with a non-unitary matrix. This recently led to exceptions being raised in the qsim tests with Cirq 1.6.0 but not with prior Cirq versions.The solution in this case turned out to be very simple: the call to
cirq.unitary(...)on line 1060 was unnecessary because the previous version ofunitary(...)simply returned the value untouched anyway. Removing the call kept the previous behavior (which was to use a non-unitary matrix in that situation) and works in both Cirq 1.5.0 and 1.6.0.(An additional change to reformatting a few lines elsewhere was needed to satisfy the CI checks for formatting.)