-
Notifications
You must be signed in to change notification settings - Fork 606
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
Supporting counts from raw samples #2686
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2686 +/- ##
=======================================
Coverage 99.61% 99.61%
=======================================
Files 255 255
Lines 20884 20913 +29
=======================================
+ Hits 20804 20833 +29
Misses 80 80
Continue to review full report at Codecov.
|
Hey @josh146, can you please re-launch the tests? |
Thanks @josh146 ! Sorry, I didn't think of checking the tests as well. Can you please relaunch? |
Thanks, @josh146 ! |
Thanks @theodotk! Someone from the team will have a look shortly :) |
Hey @Jaybsoni @antalszava could you please review the PR? |
Hey @theodotk, Thanks for the contribution, I will take a look at it today! I think @antalszava will take a closer look at it at some point once he is back in the office as he is overlooking the interview process. Thanks for you patience. Cheers, |
Co-authored-by: antalszava <[email protected]>
Hi @theodotk, it seems that changes made in the Autograd interface would be required for other interfaces too because they too cannot interpret dictionaries (e.g., trying to set If it doesn't seem to come around, no worries, I could help here tomorrow or latest next week. 👍 |
Hi @antalszava ! I fixed the problem with jax, and both tensorflow and torch return expected result with the new version. The problem arose because jax does not handle non-numeric types such as string. And in case of state vector sampling, keys of the counts dictionary are strings (because str is hashable and because converting binary to decimal instead is a less user-friendly option). But the dictionary itself seems to be handled okay. Just in case, I chose interface by setting |
Hi @theodotk, that's awesome! 🙂 It would be nice then to have a simple example for each interface, e.g., import pennylane as qml
dev = qml.device('default.qubit', wires=3, shots=10)
@qml.qnode(dev, inteface="jax")
def circuit():
return qml.sample(counts=True)
circuit() and then this PR should be looking great! 🌟 |
@antalszava you mean in the docs? |
@theodotk apologies! I meant such examples as test cases so that we can be sure things are and stay okay. 👍 |
@antalszava done! |
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 @theodotk! 🥇 Great one with getting each detail right here, thanks for addressing the comments! 🙂
Made a couple of small changes to test docstrings and names, but will merge this soon.
Thanks @antalszava , it is pleasant to see such a diligent review process! |
Context:
Fixing #2562
Description of the Change:
Adding the possibility to group samples into counts providing
counts=True
flag inqml.sample
function. This required creation of a new measurement type Counts inmeasurements.py
.The implementation is based on editing the
sample
function in_qubit_device.py
, though it was possible to base it onestimate_probability
function (or to create something new that seems superfluous to me). Choice of editing thesample
function seemed more natural for this task, but please tell me if that's not the best approach.Examples
From the issue:
It works also for observables:
Benefits:
Better visibility of the sampling result.
Possible Drawbacks:
Shouldn't be, as the change scope was minimal.
Related GitHub Issues:
2562