Skip to content
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

Adding known Quantum channels #766

Merged
merged 29 commits into from
Aug 29, 2020
Merged

Adding known Quantum channels #766

merged 29 commits into from
Aug 29, 2020

Conversation

AroosaIjaz
Copy link
Contributor

Context:

ADR 240: Adding quantum channels to PennyLane

Description of the Change:

This PR adds known noisy channels, provides corresponding tests, and updates the introduction page on PennyLane operations.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really neat @AroosaIjaz! Note that I haven't approved/requested changes since this is still a draft PR

doc/introduction/operations.rst Show resolved Hide resolved
doc/introduction/operations.rst Show resolved Hide resolved
doc/introduction/operations.rst Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/operation.py Outdated Show resolved Hide resolved
pennylane/operation.py Outdated Show resolved Hide resolved
pennylane/ops/__init__.py Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
tests/ops/test_channel_ops.py Outdated Show resolved Hide resolved
tests/ops/test_channel_ops.py Outdated Show resolved Hide resolved
class TestChannels:
"""Tests for the quantum channels"""

@pytest.mark.parametrize("ops", ch_list)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good!

I would suggest performing the tests for more than one parameter value, which can be easily done with parametrize and defining the expected outputs outside of the test themselves

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 🚀 Nice one @AroosaIjaz

Something seems to be going on with the probability tests.

@AroosaIjaz AroosaIjaz marked this pull request as ready for review August 21, 2020 19:20
@AroosaIjaz
Copy link
Contributor Author

This is ready for a final review :)

@AroosaIjaz AroosaIjaz added the review-ready 👌 PRs which are ready for review by someone from the core team. label Aug 24, 2020
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AroosaIjaz I suggested fixing some of the merge issues due to #760 being merged, before this is ready for final review!

@AroosaIjaz
Copy link
Contributor Author

@AroosaIjaz I suggested fixing some of the merge issues due to #760 being merged, before this is ready for final review!

Thank you @josh146, I am on it! :)

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AroosaIjaz! Nice to be able to use channels 😁

pennylane/ops/__init__.py Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
"AmplitudeDamping",
"GeneralizedAmplitudeDamping",
"PhaseDamping",
"DepolarizingChannel",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future additions could be bit flip noise 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add it in this PR too. What do you say? It was taken out in the ADR review process so I did not add it.

tests/ops/test_channel_ops.py Outdated Show resolved Hide resolved
tests/ops/test_channel_ops.py Outdated Show resolved Hide resolved
tests/ops/test_channel_ops.py Outdated Show resolved Hide resolved
@trbromley
Copy link
Contributor

I think the tests aren't working due to line 887 in operation.py:
if any(p > 1 for p in params):, since it doesn't like comparing p being complex to 1 (an int). Maybe we could have another check:
if any(np.iscomplex(p) for p in params): ...

AroosaIjaz and others added 8 commits August 25, 2020 11:34
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

Merging #766 into master will increase coverage by 0.09%.
The diff coverage is 98.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #766      +/-   ##
==========================================
+ Coverage   93.42%   93.52%   +0.09%     
==========================================
  Files         115      116       +1     
  Lines        7044     7099      +55     
==========================================
+ Hits         6581     6639      +58     
+ Misses        463      460       -3     
Impacted Files Coverage Δ
pennylane/operation.py 58.37% <0.00%> (ø)
pennylane/ops/__init__.py 100.00% <100.00%> (ø)
pennylane/ops/channel.py 100.00% <100.00%> (ø)
pennylane/utils.py 32.62% <0.00%> (ø)
pennylane/wires.py 98.33% <0.00%> (ø)
pennylane/_queuing.py 86.04% <0.00%> (ø)
pennylane/ops/qubit.py 96.90% <0.00%> (ø)
pennylane/qnodes/rev.py 97.95% <0.00%> (ø)
pennylane/qnodes/base.py 98.61% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd1ad5a...a847bd7. Read the comment docs.

pennylane/operation.py Outdated Show resolved Hide resolved
@AroosaIjaz
Copy link
Contributor Author

@josh146 I might need your help merging this as the codecov is not responding. Thank you!

After this is merged, I will make the final merge checks on 778 and merge that in too :)

@josh146 josh146 merged commit d7802b0 into master Aug 29, 2020
@josh146 josh146 deleted the channel_examples branch August 29, 2020 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants