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

[Code Together] Add support for qml.ResetError in default.mixed #1541

Merged
merged 35 commits into from
Aug 19, 2021

Conversation

wongwsvincent
Copy link
Contributor

@wongwsvincent wongwsvincent commented Aug 17, 2021

Context: Add support for qml.ResetError for default.mixed device, and a test of the gradient computation

Description of the Change: Added ResetError to the list of supported operations, and a test function for gradient check

Benefits: Support single-qubit reset error channel that simulates the reset of qubits to 0 or 1 given a probability.

Possible Drawbacks:

Related GitHub Issues: 1530

@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #1541 (c482b52) into master (a9a7311) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1541   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files         188      188           
  Lines       13411    13411           
=======================================
  Hits        13297    13297           
  Misses        114      114           
Impacted Files Coverage Δ
pennylane/devices/default_mixed.py 100.00% <ø> (ø)

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 a9a7311...c482b52. Read the comment docs.

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.

Hi @wongwsvincent, thank you for your contribution! 🙂 🎉 This is overall in great shape already, leaving some suggestions to refine it.

What would additionally remain, is adding tests for applying the channel using default.mixed. The gradient test is already nice to have, these additional tests would just add an extra layer of unit testing.

@@ -121,6 +121,9 @@
an owner of the pruned tensor and its constituent observables, but leaves the
original tensor in the queue without an owner.

* The `qml.ResetError` is now supported for `default.mixed` device.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 🙂 Could you add yourself to the list of contributors too on line 137?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I have just added my name.

@@ -279,6 +279,40 @@ def test_p0_p1_sum_not_normalized(self):
with pytest.raises(ValueError, match="must be between"):
channel.ResetError(1.0, 1.0, wires=0).kraus_matrices

@pytest.mark.parametrize("angle", np.linspace(0, 2 * np.pi, 7))
def test_grad_reseterror(self, angle, tol):
"""Test that analytical gradient is computed correctly for different states. Channel
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth adjusting the docstring here, as we're falling back to finite differences. qml.ResetError doesn't support analytic gradients at the moment, grad_method = "F" here denotes this fact.

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 have removed the word analytical. I hope it looks good now

@@ -91,6 +91,7 @@ class DefaultMixed(QubitDevice):
"DepolarizingChannel",
"BitFlip",
"PhaseFlip",
"ResetError",
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome catch! 🥇

@wongwsvincent
Copy link
Contributor Author

Hi @antalszava , I have made the changes as requested. This PR is ready for another review.

@antalszava
Copy link
Contributor

Hi @wongwsvincent, thanks! It seems that adding tests for applying the channel using default.mixed would still remain.

Also, there seems to be a conflict in the changelog. This can be resolved by pulling master (git checkout master followed by git pull), switching to the feature branch (git checkout add_reseterror_support) and then merging master (git merge master). The culprit seems to be the changelog file. We'd like to have all the entries kept there, for some reason git could not resolve the addition there well.

@wongwsvincent
Copy link
Contributor Author

Hi @antalszava , thanks for reviewing and catching the missing tests! I have added a few tests for qml.ResetError in tests/devices/test_default_mixed.py. I have also fixed the conflicts in CHANGELOG.md.

All checks look good without complaints. It should be ready for review.

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.

Hi @wongwsvincent, the additional tests are looking awesome, thank you! 💯 There seem to be small things to be adjusted. Overall, however, this is near approval.

@@ -160,13 +160,6 @@ def circuit(weights, ranges=None):
weights = np.random.randn(1, 2, 3)
circuit(weights, ranges=[0])

def test_id(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @wongwsvincent how come this test was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops, sorry! I must have mistakenly removed it when I was resolving my merge conflicts. The test is added back.

tests/ops/test_channel_ops.py Outdated Show resolved Hide resolved
tests/devices/test_default_mixed.py Show resolved Hide resolved
.github/CHANGELOG.md Outdated Show resolved Hide resolved
wongwsvincent and others added 3 commits August 18, 2021 14:46
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
@wongwsvincent
Copy link
Contributor Author

Hi @wongwsvincent, the additional tests are looking awesome, thank you! 💯 There seem to be small things to be adjusted. Overall, however, this is near approval.

Thanks for the suggestions @antalszava ! All the issues are addressed. Ready for a (hopefully) final review.

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! 🎉 Great one @wongwsvincent, thank you for your contribution! 🙂

@antalszava antalszava merged commit a0d3606 into PennyLaneAI:master Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants