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

Feature/implement probability mp.process counts #4943 #4952

Conversation

PietropaoloFrisoni
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni commented Dec 15, 2023

Pull request - issue #4943

Context: This pull request implements the new feature described in issue #4943 (internal assignment).

Description of the Change:
Implemented the method process_counts in the ProbabilityMP class to compute a probability measurement from a counts dictionary.

Benefits:
Representation of store sampling information in the form of a dictionary counts is much more condensed and memory efficient.

Possible Drawbacks:
The new method should reproduce the results of process_samples, implemented in the same class. It has been tested extensively, although I still need to become an expert in quantum computing and circuits.
The function checks that the provided sampling information is a dictionary matching the format returned by CountsMP.

Related GitHub Issues:
#4943


Further details:

First of all, congratulations on the library and your fantastic work!

I had the time to explore and play with the code, which is quite amazing. I hope to be considered worthy and help you maintain, expand, and improve it.

I am confident you have improvements and interesting suggestions regarding my pull request. I left some questions for you below as a comment on this PR.

Since this is an internal assignment (for which I was given a deadline), I include some information below that might be relevant for your evaluation (time zone: GMT-5):

  • Start time: December 15th, 9:07 AM
  • Marked as ready for review on: December 18th, 3:53 AM
  • Deadline: December 29th, 9:07 AM

I am incredibly grateful for your time, and I hope to learn so much from all of you!
I wish you all the best.

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (68a097c) 99.50% compared to head (d0bfeaa) 99.67%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4952      +/-   ##
==========================================
+ Coverage   99.50%   99.67%   +0.16%     
==========================================
  Files         390      392       +2     
  Lines       35585    35288     -297     
==========================================
- Hits        35409    35173     -236     
+ Misses        176      115      -61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PietropaoloFrisoni PietropaoloFrisoni marked this pull request as ready for review December 18, 2023 08:52
@PietropaoloFrisoni
Copy link
Contributor Author

As I mentioned above, I have a few elementary questions for you:

  • I implemented a few checks on the counts parameter to verify that it is a dictionary matching the format returned by class:CountsMP, defined in pennylane.measurements.counts.py. I noticed that there are no similar checks in other methods. Is this a deliberate choice? If that's the case, please tell me if I should remove such checks (or if I should add more of them).
  • I understand that the sampling information should be provided so that keys correspond to measurement outcomes in binary strings, like 00010, etc., and no other digit is allowed. Please let me know if that's not the case, and I'll change the code.
  • Finally, just a comment. Since I pushed the last changes, the test associated with codecov/project seems stuck, although it was perfectly fine in the previous commit(s). I suspect it is most probably a codecov issue.

If you want to test the code and don't want to write something from scratch, I leave you a very basic snippet I used during some of my tests.

I wish you all the best once again!

@qml.qnode(qml.device("default.qubit"))
def circuit():
    qml.Hadamard(wires=1)
    qml.Hadamard(wires=2)
    qml.CNOT(wires=[0, 1])
    return qml.sample(wires=(0, 1, 2))


wires = (0, 1)
wire_order = qml.wires.Wires((1, 0))

samples = circuit(shots=100)
counts = {}

for i in range(samples.shape[0]):
    binary_string = "".join(map(str, samples[i, :]))
    counts[binary_string] = counts.get(binary_string, 0) + 1

print(f"samples = {samples}")
print(f"counts = {counts}\n")

prob_old = qml.probs(wires=wires).process_samples(samples, wire_order=wire_order)
prob_new = qml.probs(wires=wires).process_counts(counts, wire_order=wire_order)
(prob_old == prob_new).all()

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.

Sorry about the delay in getting back to you.

This is looking good 🎉

I left two minor comments, but other than that, this is exactly what I was looking for.

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/measurements/probs.py Outdated Show resolved Hide resolved
Commit suggestion from Christina (removing specification about the internal assignment)

Co-authored-by: Christina Lee <chrissie.c.l@gmail.com>
@PietropaoloFrisoni
Copy link
Contributor Author

Hi @albi3ro and @mudit2812.

Thank you so much for your review and your time!

Sorry about the delay in getting back to you.

No worries, I didn't want to bother you with reminders (especially during Christmas time)

This is looking good 🎉

I left two minor comments, but other than that, this is exactly what I was looking for.

Thank you, Christina!

I pushed a commit with your suggestion and removed the asserts as you both suggested.

If you are not ready to approve the PR, please tell me if there's something else you want me to modify.

Merry Christmas to both of you, as well as to everyone else on the team!

@mudit2812
Copy link
Contributor

Regarding the test failure showing up right now. It's unrelated to your PR @PietropaoloFrisoni and there's no need to worry about it. The test validates a finite-shots workflow where the results are non-deterministic. I will resolve this in a different PR. In the mean time, you can re-run CI without having to make new changes by running git commit --allow-empty -m 'Trigger CI'; git pull; git push

Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

These changes look good to me. I'm happy to approve once CI is passing.

Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Congratulations @PietropaoloFrisoni ! Everything is passing, and the changes look good to me. Happy to get this in.

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 great!

I hope you've enjoyed the process and the change to learn a little about pennylane 🎉

@albi3ro albi3ro merged commit 9030395 into PennyLaneAI:master Dec 20, 2023
35 checks passed
@PietropaoloFrisoni PietropaoloFrisoni deleted the feature/Implement-ProbabilityMP.process_counts-#4943 branch December 21, 2023 07:13
mudit2812 pushed a commit that referenced this pull request Jan 19, 2024
### Pull request - issue #4943

**Context: This pull request implements the new feature described in
issue #4943 (internal assignment).**

**Description of the Change:**
Implemented the method `process_counts` in the `ProbabilityMP` class to
compute a probability measurement from a counts dictionary.

**Benefits:**
Representation of store sampling information in the form of a dictionary
counts is much more condensed and memory efficient.

**Possible Drawbacks:** 
The new method should reproduce the results of `process_samples`,
implemented in the same class. It has been tested extensively, although
I still need to become an expert in quantum computing and circuits.
The function checks that the provided sampling information is a
dictionary matching the format returned by `CountsMP`.

**Related GitHub Issues:** 
#4943 


----------------------------------------------------------------------------------------------------------------------------------------------------------------------

**Further details:** 

First of all, congratulations on the library and your fantastic work! 

I had the time to explore and play with the code, which is quite
amazing. I hope to be considered worthy and help you maintain, expand,
and improve it.

I am confident you have improvements and interesting suggestions
regarding my pull request. I left some questions for you below as a
comment on this PR.

Since this is an internal assignment (for which I was given a deadline),
I include some information below that might be relevant for your
evaluation (time zone: GMT-5):

- *Start time:* December 15th, 9:07 AM 
- *Marked as ready for review on:* December 18th, 3:53 AM
- *End time:* TBD
- *Deadline:* December 29th, 9:07 AM

I am incredibly grateful for your time, and I hope to learn so much from
all of you!
I wish you all the best.

---------

Co-authored-by: Christina Lee <chrissie.c.l@gmail.com>
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

3 participants