-
Notifications
You must be signed in to change notification settings - Fork 50
Fix dynamic wires with samples #1321
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
base: v0.44.0_rc
Are you sure you want to change the base?
Conversation
|
Hello. You may have forgotten to update the changelog!
|
This comment has been minimized.
This comment has been minimized.
dime10
left a comment
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.
Thank you @rniczh !
pennylane_lightning/core/simulators/lightning_qubit/catalyst/LightningSimulator.cpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/simulators/lightning_qubit/measurements/MeasurementsLQubit.hpp
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.44.0_rc #1321 +/- ##
==============================================
- Coverage 96.01% 93.89% -2.13%
==============================================
Files 307 243 -64
Lines 46923 40264 -6659
==============================================
- Hits 45055 37804 -7251
- Misses 1868 2460 +592
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
maliasadi
left a comment
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.
--------- Co-authored-by: Ali Asadi <[email protected]>
d3abf70 to
6e9f177
Compare
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.
Nice work Hongsheng! Just a few questions and comments. Is there any reason we're implementing this for nvm, just saw Ali already addressed thislightning.qubit but not the other devices? Does this issue exist for the others as well?
pennylane_lightning/core/simulators/lightning_qubit/catalyst/LightningSimulator.cpp
Show resolved
Hide resolved
pennylane_lightning/core/simulators/lightning_qubit/catalyst/LightningSimulator.cpp
Show resolved
Hide resolved
pennylane_lightning/core/simulators/lightning_gpu/catalyst/LightningGPUSimulator.cpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/simulators/lightning_gpu/catalyst/LightningGPUSimulator.cpp
Outdated
Show resolved
Hide resolved
| *(this->device_sv)}; | ||
| } | ||
|
|
||
| void LightningGPUSimulator::CompactStateVector() { |
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.
Where are these tested? I couldn't find any tests for these methods for all devices.
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.
The function is triggered by getMeasurements(), ensuring that any test executing the measurement process also updates the state vector. Currently, only one test case is added (the one that reported from the issue PennyLaneAI/catalyst#2339). The specific test can be found here:
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.
Will this be added to Catalyst for the release?
per our offline discussions, what if these aux wires are entangled, we should test and add some handling mechanism for these cases in both Lightning & Catalyst?
pennylane_lightning/core/simulators/lightning_gpu/catalyst/LightningGPUSimulator.cpp
Outdated
Show resolved
Hide resolved
| auto GenerateSamples(size_t shots) -> std::vector<size_t>; | ||
|
|
||
| // Compact state vector by removing released qubits | ||
| void CompactStateVector(); |
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.
minor: this is the style we've been following for a while:
| void CompactStateVector(); | |
| void compactStateVector(); |
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.
Should we call this reducedStateVector instead of compact?
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.
Sure! I updated the name here: a5e2132
|
Could you also bump the rc version to 3 within |
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Sure abd373d |
jzaia18
left a comment
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.
Looking good, just 1 last comment
pennylane_lightning/core/simulators/lightning_qubit/catalyst/LightningSimulator.cpp
Outdated
Show resolved
Hide resolved
dime10
left a comment
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.
Nice work! Let's get this across the finish line to make sure the dynamic allocation feature actually works :)
Only one question to resolve really:
| // maintained, | ||
| // in particular: measurements must not compute results for released | ||
| // qubits, only for active/allocated qubits |
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.
We can remove the TODO now :)
| * @param data State vector data to normalize. | ||
| */ | ||
| template <class ComplexT, class Alloc = std::allocator<ComplexT>> | ||
| inline void normalizeStateVector(std::vector<ComplexT, Alloc> &data) { |
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.
Just wanted to mention I believe most devices already have a normalize method, since it's needed for the measure implementation:
pennylane-lightning/pennylane_lightning/core/simulators/lightning_qubit/StateVectorLQubit.hpp
Line 908 in 4739a0f
| void normalize() { |
| // TODO: We don't have to check whether the released qubit is pure, but it | ||
| // is something we can add easily to catch program errors. |
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.
We should get consensus on the desired behaviour. I don't think this needs to be a big thing, just picking one of the options and checking with product what they think. As far as I know the options are:
- (1) it is enforced that qubits must be pure (disentangled) before release
- implementation: (simulator-only) raise an error if the user releases an entangled qubit
- benefit: nice to catch (likely) programming errors for the user, no undefined or non-deterministic behaviour
- issues:
- if an entire subset of qubits is released at once, it should be okay if they are entangled with each other, but here we do the release one-by-one so that complicates the detection
- takes a bit of work each qubit release
- (2) it is well-defined what happens when qubits are not pure:
- implementation: reset affected qubits projecting the remainder of state onto of the two subspaces
- this could be random (like on hw) or a forced projection onto 0 for example (simulator-only)
- since we want the behaviour to be well defined and observable, I think this should happen immediately upon release (e.g. a snapshot would reflect this right away, but I'm not sure that even with option 3 the partial statevector would ever be observable?)
- benefit: can be deterministic still, is observable
- issues: takes a bit of work each qubit release
- implementation: reset affected qubits projecting the remainder of state onto of the two subspaces
- (3) it is undefined behaviour what happens when releasing non-pure qubits:
- implementation: undefined means we can do whatever is convenient, but at the very least we have to project (and compact) the state before measurement processes and before handing out fresh qubits that reuse previously deallocated space in the statevector
- benefit: could be faster to delay than doing something immediately like in the other options
- issues: least clean?
Although, even if the projection is done lazily we could still raise an error at that point, it might just be less precise than if we do it immediately. So maybe there is only two choices (error or silent projection), and the question of whether to do it eagerly or lazily is orthogonal to that choice. Another variant could check for an error eagerly, but compact lazily. Or another one could be have a safe release and an unsafe release, which can be toggled by the user.
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.
My gut decision would probably be option 1 for safety (and compact later, and/or safety toggle).
Context:
It get unexpected result due to the aux wires involved.
Description of the Change:
Only take the device wires for generating samplesCompact state vector before measurement.
For state vector normalization:
Benefits:
Possible Drawbacks:
Related GitHub Issues:
PennyLaneAI/catalyst#2339
[[sc-107206]]