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

Update tests to ensure that Torch backpropagation is supported #1598

Merged
merged 5 commits into from
Aug 30, 2021

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Aug 27, 2021

Context: We now support backpropagation using Torch and default.qubit, however a lot of our integration tests skipped this combination.

Description of the Change: Enables testing this combination across our test suite.

Benefits: Better test coverage.

Possible Drawbacks: There is an odd issue I noticed; return qml.probs(0), qml.probs(1) returns a (2, 2) shape array in TensorFlow, but a (4,) array in Torch+backprop. This could be due to differing behaviour in the DefaultQubitTorch._asarray() method.

Related GitHub Issues: n/a

@josh146 josh146 added the review-ready 👌 PRs which are ready for review by someone from the core team. label Aug 27, 2021
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #1598 (b763288) into master (802c45c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1598   +/-   ##
=======================================
  Coverage   99.13%   99.13%           
=======================================
  Files         195      195           
  Lines       14104    14104           
=======================================
  Hits        13982    13982           
  Misses        122      122           

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 802c45c...b763288. 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.

Looks good to me! 💯 Thank you for catching these @josh146 🎉

.github/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -2406,6 +2406,25 @@ def circuit():
# evaluated one expval altogether
assert spy.call_count == 1

def test_do_not_split_analytic_torch(self, mocker):
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps out of scope and an overall organizational question: wouldn't these tests belong to the dedicated files for each backprop device? So e.g., in test_default_qubit_torch.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good point! The other tests are already in this file, however, so it would likely be out of scope to move all tests here.

@@ -81,6 +88,9 @@ def circuit(a):

def test_execution_with_interface(self, dev_name, diff_method):
"""Test execution works with the interface"""
if diff_method == "backprop":
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this case doesn't support backprop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this test explicitly tests the interface, but backprop requires/has no interface :)


def test_jacobian_dtype(self, dev_name, diff_method, tol):
"""Test calculating the jacobian with a different datatype"""
if diff_method == "backprop":
pytest.skip("Test does not support backprop")
Copy link
Contributor

Choose a reason for hiding this comment

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

Also curious: how come this doesn't work in backprop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reason as above; setting the dtype is a feature of the interface, however backprop does not use the interface

tests/interfaces/test_qnode_torch.py Show resolved Hide resolved
Co-authored-by: antalszava <antalszava@gmail.com>
@josh146 josh146 merged commit b852798 into master Aug 30, 2021
@josh146 josh146 deleted the update-torch branch August 30, 2021 12:09
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

2 participants