-
Notifications
You must be signed in to change notification settings - Fork 586
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
Shots refactor I of III #1075
Shots refactor I of III #1075
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1075 +/- ##
=======================================
Coverage 97.72% 97.72%
=======================================
Files 154 154
Lines 11647 11658 +11
=======================================
+ Hits 11382 11393 +11
Misses 265 265 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Chase Roberts <chase@xanadu.ai>
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 good @mariaschuld! My only major comment I think was regarding warning vs. exception, the rest were all minor docstring suggestions :)
pennylane/tape/qnode.py
Outdated
raise qml.QuantumFunctionError( | ||
"The shots argument is reserved and will be overwritten!" | ||
) |
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 would make this a warning rather than an exception. I can't quite explain, but it 'feels' more like a warning.
Like, we shouldn't stop the user from creating the qnode, but we should warn them that the shots argument won't act as they expect?
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.
The reason why I changed to the error is that I played around and all sorts of strange things happened until I realised that I used a shots argument in my code and I had just overlooked the warning...
I think a warning is more appropriate but an exception safer? Happy to change though!
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
tests/tape/tapes/test_qnode.py
Outdated
assert len(circuit(0.8)) == 10 | ||
|
||
def test_shots_reserved_qfunc_argument(self): | ||
"""Tests that a warning is raised if the quantum function has a shots argument.""" |
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.
@josh146 is this the test you had in mind?
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.
Yep looks good!
tests/tape/tapes/test_qnode.py
Outdated
assert len(circuit(0.8)) == 10 | ||
|
||
def test_shots_reserved_qfunc_argument(self): | ||
"""Tests that a warning is raised if the quantum function has a shots argument.""" |
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.
Yep looks good!
[ch4089] |
@josh146 and @Thenerdstation, thanks for the comments! I made a few more changes which ensure backwards compatibility. So we have:
|
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Chase Roberts <chase@xanadu.ai>
[ 1 1 1] | ||
>>> circuit(0.8) | ||
[ 1 1 1 -1 -1 1 1 1 1 1] | ||
|
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 can't suggest it, but you need 3 backticks here.
Context:
The shots refactor is a long-planned change that makes the execution of circuits with varying numbers of shots more flexible.
The first change adds an optional kwarg to the qnode's
__call__
function which temporarily changes the number of shots that the device is executed with.Description of the Change:
Temporarily set the number of shots in qnode's
__call__
function.Add a test.
Change docs and changelog.
Benefits:
In every execution one can now overwrite the device's default shot argument.
Possible Drawbacks:
If a user creates a qfunc with a custom
shots
keyword argument used in the circuit, the kwarg will be popped from the argument list and is never fed into the circuit. This leads to unexpected behaviour, such asIn some sense, we make
shots
a reserved argument.