-
Notifications
You must be signed in to change notification settings - Fork 575
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
Adds bit and phase flip channels #954
Conversation
Codecov Report
@@ Coverage Diff @@
## master #954 +/- ##
=======================================
Coverage 97.82% 97.82%
=======================================
Files 149 149
Lines 10237 10259 +22
=======================================
+ Hits 10014 10036 +22
Misses 223 223
Continue to review full report at Codecov.
|
![:atom: :atom:](https://github.githubassets.com/images/icons/emoji/atom.png)
Since these are so urprised these weren't in the previous PR that added channels 🤔 |
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.
Looks great! Thanks for taking the initiative to do this 🚀
My only major suggestion is that I personally prefer naming them BitFlip
and PhaseFlip
pennylane/ops/channel.py
Outdated
@@ -223,6 +223,90 @@ def _kraus_matrices(cls, *params): | |||
return [K0, K1, K2, K3] | |||
|
|||
|
|||
class BitFlipChannel(Channel): |
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.
I actually like dropping the "Channel" and naming these BitFlip
and PhaseFlip
. The only case we had "Channel" in the name I believe was for DepolarizingChannel
, which makes sense since just Depolarizing
is a bit akward
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.
💯
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.
Fair enough! How do you feel about BitFlipError
and PhaseFlipError
? I'd like to have something to denote that they are error channels and not just regular operations (also I just noticed I that this is what I called them in the changelog... 🤦)
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.
I have this discussion with Alain all the time...
I personally prefer names to be implicit and short, rather than explicit and long. So while you're right that a "BitFlip" could potentially be interpreted as a regular operation (apply PauliX), it will likely be clear from context and from a good docstring that this is not the case. The more compact name then leads to cleaner and shorter code.
tests/ops/test_channel_ops.py
Outdated
assert np.allclose(op(0, wires=0).kraus_matrices[0], np.eye(2), atol=tol, rtol=0) | ||
assert np.allclose(op(0, wires=0).kraus_matrices[1], np.zeros((2, 2)), atol=tol, rtol=0) | ||
|
||
def test_p_arbitrary(self, tol): |
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.
This is fine, but it's good practice to run tests for several parameter values. This is usually done using the parametrize decorator
@pytest.mark.parametrize("p", [0.1, 0.4, 0.7])
def test_p_arbitrary(self, p, tol)
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.
Good call, this allows for merging the two separate tests into a single parameterized test.
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
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.
![:atom: :atom:](https://github.githubassets.com/images/icons/emoji/atom.png)
Context: Bit flips and phase flips are commonly occurring errors. As we begin doing more noisy simulations with the
default.mixed
device, it will be convenient to have these error channels available.Description of the Change: Adds two new error channels,
BitFlipChannel
andPhaseFlipChannel
(and associated documentation and tests).Benefits: Adds to a growing library of errors to make noisy simulations more flexible.
Possible Drawbacks: None
Related GitHub Issues: None