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

Fix bug in tracking the number of device executions #1046

Merged
merged 3 commits into from
Jan 28, 2021

Conversation

mariaschuld
Copy link
Contributor

@mariaschuld mariaschuld commented Jan 28, 2021

Context

With PR #1008 we are creating situations in which the tape is executed on a different device than the user has given to the qnode. This is a design issue we should access in the long term. In the short term it introduces a bug because the num_executions attribute is no longer tracked correctly on the user's device. This PR fixes the issue by manually adding 1 to device.num_executions if we are swapping out devices.

It also adds some tests that would have caught the bug.

PS: Ignore the branch name!

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

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.

All comments are minor :)

pennylane/tape/qnode.py Outdated Show resolved Hide resolved
tests/tape/tapes/test_qnode.py Outdated Show resolved Hide resolved
# actually run so she has full control. This could be done by changing the class
# of the user's device before and after executing the tape.
if self.device != self._original_device:
self._original_device._num_executions += 1 # pylint: disable=protected-access
Copy link
Member

Choose a reason for hiding this comment

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

This might sound crazy, but will self.qtape.execute(device=self.device) always result in just 1 additional execution?

Or is this a case where there could be more than 1 under the scenes?

I suppose I am being crazy

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 was wondering too. In future this may very well happen, but I think it does not happen at the moment. More reason to find a better fix one day.

Copy link
Member

Choose a reason for hiding this comment

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

In that case you could do

diff_executions = self.device.num_executions
res = self.qtape.execute(device=self.device)
diff_executions = self.device.num_executions - diff_executions
if self.device is not self._original_device:
    self._original_device._num_executions += diff_executions

return qml.expval(qml.PauliZ(0))

dev = qml.device("default.qubit", wires=2)
qn = QNode(func, dev, interface="autograd")
Copy link
Member

Choose a reason for hiding this comment

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

Should we add diff_method="backprop" here to be explicit, in case we ever change the logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if we change the logic and don't switch the device any more, we actually don't only want to test "backprop" either. Could do a fixture with all diff methods?

mariaschuld and others added 2 commits January 28, 2021 10:09
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #1046 (524f09c) into master (c1bfc27) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1046   +/-   ##
=======================================
  Coverage   97.94%   97.94%           
=======================================
  Files         153      153           
  Lines       11481    11483    +2     
=======================================
+ Hits        11245    11247    +2     
  Misses        236      236           
Impacted Files Coverage Δ
pennylane/tape/qnode.py 98.44% <100.00%> (+0.01%) ⬆️

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 c1bfc27...524f09c. Read the comment docs.

@mariaschuld mariaschuld merged commit 2a79ae0 into master Jan 28, 2021
@mariaschuld mariaschuld deleted the override_device_class branch January 28, 2021 08:30
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

2 participants