Patch Measurements.probs(wires) to Measurements.probs() when called with all wires #744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maliasadi
changed the title
Measurements.probs to fix the issue with patching qml.probs to the proper C++ implementations
maliasadi
commented
May 28, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #744 +/- ##
==========================================
- Coverage 94.07% 91.98% -2.10%
==========================================
Files 102 66 -36
Lines 14836 9041 -5795
==========================================
- Hits 13957 8316 -5641
+ Misses 879 725 -154 ☔ View full report in Codecov by Sentry. |
pennylane_lightning/core/src/simulators/lightning_kokkos/measurements/MeasurementsKokkos.hpp
Outdated
Show resolved
Hide resolved
AmintorDusko
approved these changes
May 29, 2024
There was a problem hiding this comment.
As I said before, we have proper Python bindings for probs(). The problem is that we always provide wires in the devices' class even if this is an all-wires situation. The more straightforward solution would be to have a test somewhere (like you did in this PR) and redirect when possible. The best place for that is in the C++ layer. This is a cheap check. I'm happy to approve.
vincentmr
reviewed
May 29, 2024
There was a problem hiding this comment.
Thanks @maliasadi , just a couple questions but I'm basically ready to approve. Nice formatting and docstring edits.
pennylane_lightning/core/src/simulators/lightning_kokkos/measurements/MeasurementsKokkos.hpp
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_kokkos/measurements/MeasurementsKokkos.hpp
Outdated
Show resolved
Hide resolved
…rements/MeasurementsKokkos.hpp Co-authored-by: Vincent Michaud-Rioux <[email protected]>
vincentmr
approved these changes
May 29, 2024
5 tasks
maliasadi
added a commit
that referenced
this pull request
Jun 17, 2024
### Before submitting
Please complete the following checklist when submitting a PR:
- [ ] All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to
the
[`tests`](../tests) directory!
- [x] All new functions and code must be clearly commented and
documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running `make docs`.
- [x] Ensure that the test suite passes, by running `make test`.
- [ ] Add a new entry to the `.github/CHANGELOG.md` file, summarizing
the
change, and including a link back to the PR.
- [x] Ensure that code is properly formatted by running `make format`.
When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
------------------------------------------------------------------------------------------------------------
**Context:**
PR #744 patches `probs(wires=[])` to calculate the probabilities of all
wires in LightningKokkos. This PR fixes the issue to keep the
consistency among all Lightning devices as well as the integration with
Catalyst.
**Description of the Change:**
**Benefits:**
Consistent behaviour among all devices for calculating `probs(wires=[])
= [1.0]`.
**Possible Drawbacks:**
**Related GitHub Issues:**
---------
Co-authored-by: ringo-but-quantum <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before submitting
Please complete the following checklist when submitting a PR:
All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to the
testsdirectory!All new functions and code must be clearly commented and documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running
make docs.Ensure that the test suite passes, by running
make test.Add a new entry to the
.github/CHANGELOG.mdfile, summarizing thechange, and including a link back to the PR.
Ensure that code is properly formatted by running
make format.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context:
All calls to
qml.probsto calculate the probs of the whole system in the analytic mode, patches to the C++Measurements.probs(wires, ...)method that's implemented for computing the probabilities of a subset of wires. There is a more dedicated method (Measurements.probs()) that skips part of the logic inMeasurements.probs(wires)which is indeed required to deal with subsystems. We should callMeasurements.probs()when computing the probs of the whole system instead. This PR ensures that we patch toMeasurements.probs()resulting a ~12% performance boost for qubits > 10.Description of the Change:
Measurements.probs(wires, ...)method to callMeasurements.probs()in LQ and LK when requesting the probs of all wires.Benefits:
This will skip the unnecessary steps required for calculating the probs of a subsystem by calling the proper C++
probsimplementation in LQ and LK.Possible Drawbacks:
Alternatively, we could fix this by calling to the proper
probsC++ implementation in Python Bindings instead. I need to do a bit of investigation to find the better solution here. Any thoughts would be appreciated :)Related GitHub Issues:
[sc-63167]
[sc-64514]