-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add _apply_channel_ optimizations for reset and confusion #5917
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at the details yet, but this is what I was hoping we would do! Thank you, Dax!
I have one concern. When we do things this way, i.e. when we have different code paths implementing the same operation to be employed under different circumstances, we should test that the code paths agree on a few test inputs. We do this for _unitary_
and _apply_unitary_
using this function. By analogy, we should have assert_has_consistent_apply_channel
.
In fact, if you look at our present consistency checks for channels, there are surprisingly few. In fact, we have an issue - #4579 - about adding more (albeit the issue is about consistency between different representations such as Choi/Kraus/etc and doesn't mention _apply_channel_
explicitly).
How do you feel about adding a assert_has_consistent_apply_channel
that would verify that _kraus_
and _apply_channel_
yield the same results? (I'd do it in a separate PR to be merged before this one so that we could make use of the new assert helper in here, but that's up to you of course.)
+1 I had the same concern. Thanks for the suggestion on the approach. I will try to do it as part of this PR. If it gets hairy I will open a separate PR for it. |
Done. I have it calculate the superoperator from the Kraus, and separately calculating the superoperator by calling apply_channel on the identity tensor, and then comparing the results of each. I also added an apply_channel for phase_damp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
return 1 | ||
|
||
with pytest.raises(AssertionError): | ||
cirq.testing.assert_has_consistent_apply_channel(NoKraus()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: I think it'd be nice to make this case work by having protocol.kraus
compute the Kraus representation when _apply_channel_
is provided but _kraus_
isn't. Probably a good idea for a follow-up rather than this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #5921
…#5917) Confusion matrix and reset channels both can be viewed as starting with a zero DM tensor and then copying (adding) in scaled slices from the original DM tensor. Thus we make a helper function that does this and add `_apply_channel_` optimizations to those gates. Fixes quantumlib#5901. Starts quantumlib#5900 though I haven't looked at all gates. Also starts quantumlib#4579 but there's likely more to do there as well. I didn't add the new test function to the primary test suite because creating superoperators is likely computationally expensive (granted most if not all gates that use this would be three or fewer qubits, which is still cheap), and the test not relevant for most gates.
…#5917) Confusion matrix and reset channels both can be viewed as starting with a zero DM tensor and then copying (adding) in scaled slices from the original DM tensor. Thus we make a helper function that does this and add `_apply_channel_` optimizations to those gates. Fixes quantumlib#5901. Starts quantumlib#5900 though I haven't looked at all gates. Also starts quantumlib#4579 but there's likely more to do there as well. I didn't add the new test function to the primary test suite because creating superoperators is likely computationally expensive (granted most if not all gates that use this would be three or fewer qubits, which is still cheap), and the test not relevant for most gates.
Confusion matrix and reset channels both can be viewed as starting with a zero DM tensor and then copying (adding) in scaled slices from the original DM tensor. Thus we make a helper function that does this and add
_apply_channel_
optimizations to those gates.Fixes #5901.
Starts #5900 though I haven't looked at all gates.
Also starts #4579 but there's likely more to do there as well. I didn't add the new test function to the primary test suite because creating superoperators is likely computationally expensive (granted most if not all gates that use this would be three or fewer qubits, which is still cheap), and the test not relevant for most gates.