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 cases for qml.sample(..., counts=True) #2839

Merged
merged 21 commits into from Jul 26, 2022
Merged

Fix cases for qml.sample(..., counts=True) #2839

merged 21 commits into from Jul 26, 2022

Conversation

antalszava
Copy link
Contributor

@antalszava antalszava commented Jul 20, 2022

Context:
In a recent PR it was found that the qml.shots(..., counts=True) behaves in unexpected ways in some cases. The original addition was made in this PR.

Description of the Change:

Fixed the use of returning counts when:

  • For varying number of wires (other than 3);
  • Using the interfaces;
  • Counts are returned as part of multiple measurements.

Note that counts now no longer return tensor objects, but rather raw dictionaries or sequences that contain dictionaries.

Benefits:

Returning counts works in more cases as expected.

Possible Drawbacks:
N/A

Related GitHub Issues:
N/A

@antalszava antalszava added the bug 🐛 Something isn't working label Jul 20, 2022
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #2839 (d034c72) into master (14485da) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #2839    +/-   ##
========================================
  Coverage   99.63%   99.63%            
========================================
  Files         251      252     +1     
  Lines       20572    20757   +185     
========================================
+ Hits        20496    20681   +185     
  Misses         76       76            
Impacted Files Coverage Δ
pennylane/measurements.py 99.63% <ø> (ø)
pennylane/ops/qubit/observables.py 100.00% <ø> (ø)
pennylane/_qubit_device.py 99.45% <100.00%> (+<0.01%) ⬆️
pennylane/drawer/tape_mpl.py 100.00% <100.00%> (ø)
pennylane/drawer/tape_text.py 100.00% <100.00%> (ø)
pennylane/interfaces/autograd.py 100.00% <100.00%> (ø)
pennylane/interfaces/jax.py 100.00% <100.00%> (ø)
pennylane/interfaces/tensorflow.py 100.00% <100.00%> (ø)
pennylane/interfaces/torch.py 100.00% <100.00%> (ø)
pennylane/ops/op_math/__init__.py 100.00% <100.00%> (ø)
... and 2 more

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 52d656d...d034c72. Read the comment docs.

@antalszava antalszava changed the title Fix counts Fix cases for qml.sample(..., counts=True) Jul 20, 2022
@antalszava antalszava requested a review from rmoyard July 20, 2022 21:24
@antalszava antalszava marked this pull request as ready for review July 20, 2022 21:25
@antalszava antalszava requested a review from albi3ro July 21, 2022 13:05
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Great changes @antalszava, I am glad that it is working as expected 💯 I've left some comments. One thing that is out of scope of this PR is that I would find great that we always return the dictionary for every state:

{'001': 7, '111': 3} could become {'000': 0, 001': 7, '010': 0, '011': 0, '100': 0, '101': 0, '110': 0 '111': 3}

pennylane/_qubit_device.py Outdated Show resolved Hide resolved
pennylane/_qubit_device.py Outdated Show resolved Hide resolved
pennylane/_qubit_device.py Show resolved Hide resolved
@antalszava
Copy link
Contributor Author

@rmoyard thanks for the comments!

One thing that is out of scope of this PR is that I would find great that we always return the dictionary for every state:{'001': 7, '111': 3} could become {'000': 0, 001': 7, '010': 0, '011': 0, '100': 0, '101': 0, '110': 0 '111': 3}

Good point! Could it be user-dependent, i.e., some prefer this some prefer that? Would it be worth having kwarg for it?

@albi3ro
Copy link
Contributor

albi3ro commented Jul 21, 2022

@rmoyard thanks for the comments!

One thing that is out of scope of this PR is that I would find great that we always return the dictionary for every state:{'001': 7, '111': 3} could become {'000': 0, 001': 7, '010': 0, '011': 0, '100': 0, '101': 0, '110': 0 '111': 3}

Good point! Could it be user-dependent, i.e., some prefer this some prefer that? Would it be worth having kwarg for it?

That could be done with defaultdict, like used in specs.

@antalszava antalszava requested a review from rmoyard July 22, 2022 13:52
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Left some minor, optional potential improvements.

Excellent job getting this to work!

pennylane/_qubit_device.py Show resolved Hide resolved
pennylane/_qubit_device.py Show resolved Hide resolved
pennylane/_qubit_device.py Outdated Show resolved Hide resolved
pennylane/_qubit_device.py Show resolved Hide resolved
pennylane/_qubit_device.py Outdated Show resolved Hide resolved
pennylane/interfaces/autograd.py Show resolved Hide resolved
pennylane/qnode.py Outdated Show resolved Hide resolved
antalszava and others added 5 commits July 22, 2022 18:24
Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: Christina Lee <christina@xanadu.ai>
@antalszava
Copy link
Contributor Author

[sc-23475]

@antalszava antalszava requested a review from albi3ro July 25, 2022 17:37
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks @antalszava, it looks good on my side 💯

Copy link
Contributor

@albi3ro albi3ro 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 go from my side.

@antalszava
Copy link
Contributor Author

Created an issue for returning or states with counts:
#2864

From my side, I'm still wondering about the refactor to the QuantumTape.is_sampled attribute, but happy to create an issue for that too if we have a clear vision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants