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

Allows shots to be passed to transforms at runtime #1707

Merged
merged 12 commits into from
Sep 30, 2021
Merged

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Sep 30, 2021

Context: The number of shots is currently a reserved runtime argument when calling QNodes. This PR enables this for batch transforms as well.

Description of the Change:

  • When a batch transform is called with shots as keyword argument, this is intercepted and used as a runtime argument when calling qml.execute().

  • In addition, a small bug was found and fixed at the same time, whereby the number of shots was not returned to the original value after execution.

Benefits: Transforms can also be dynamically called with different shot values.

Possible Drawbacks: n/a

Related GitHub Issues: n/a

@josh146 josh146 added the review-ready 👌 PRs which are ready for review by someone from the core team. label Sep 30, 2021
Comment on lines -69 to -70
if device.shots != original_shots:
device.shots = original_shots
Copy link
Member Author

Choose a reason for hiding this comment

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

@glassnotes this is the bug affecting the notebook - != isn't safe enough, better to simply always revert the shots.

@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #1707 (f585191) into master (8a14365) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1707   +/-   ##
=======================================
  Coverage   99.20%   99.20%           
=======================================
  Files         201      201           
  Lines       15127    15134    +7     
=======================================
+ Hits        15007    15014    +7     
  Misses        120      120           
Impacted Files Coverage Δ
pennylane/interfaces/batch/__init__.py 100.00% <100.00%> (ø)
pennylane/transforms/batch_transform.py 100.00% <100.00%> (ø)
pennylane/transforms/classical_jacobian.py 100.00% <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 8a14365...f585191. Read the comment docs.

Comment on lines +62 to +64
if shots == device.shots:
yield
return
Copy link
Member Author

Choose a reason for hiding this comment

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

Now, if shots == device.shots, we yield and do nothing!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good idea!


# the original QNode is unaffected
assert circuit(x).shape == tuple()
assert circuit(x, shots=1000).shape == tuple()
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if a circuit uses the shots argument? This was always a tricky case, but shall we define intended behaviour with a test?

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently raise a deprecation warning in the QNode, let me check if it also occurs here, or if one should be added manually 🤔

if "shots" in inspect.signature(func).parameters:

@@ -161,6 +161,7 @@ def jacobian_wrapper(*args, **kwargs):
if not hybrid:
return qjac

kwargs.pop("shots", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, your comment just made me think of a better way, allowing this to be removed!

Copy link
Contributor

@mariaschuld mariaschuld left a comment

Choose a reason for hiding this comment

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

I still have a very bad feeling about this syntax, but since we have it, it makes only sense to add it to the batch transforms!

Approving, but I would strongly recommend to improve the changelog :)

josh146 and others added 3 commits September 30, 2021 23:31
Co-authored-by: Maria Schuld <mariaschuld@gmail.com>
@josh146 josh146 merged commit 9b709c3 into master Sep 30, 2021
@josh146 josh146 deleted the shots-transforms branch September 30, 2021 18:04
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