-
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
| const std::vector<std::size_t> &wires, const std::string &kernelname, | ||
| std::size_t num_burnin, std::size_t num_samples) { | ||
| std::uniform_real_distribution<PrecisionT> distrib(0.0, 1.0); | ||
| std::size_t num_qubits = wires.size(); |
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.
Do we know that this is sufficient to identify which wires should be selected in a more complex dynamic (de)allocation scheme, or does it rely on the assumption that the selected wires are always the bottom n wires?
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.
Mentioned this offline but I think we'd be better off providing a universal solution that we never got around to implementing (see #1254 (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.
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.
Wow that was fast! 🤩
@maliasadi are you able to look this over?
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% 89.22% -6.80%
==============================================
Files 307 187 -120
Lines 46923 29012 -17911
==============================================
- Hits 45055 25886 -19169
- Misses 1868 3126 +1258
☔ 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?
| // get device wires | ||
| auto &&dev_wires = getDeviceWires(wires); | ||
|
|
||
| auto li_samples = this->GenerateSamples(device_shots); | ||
|
|
||
| // Get device wires | ||
| auto &&dev_wires = getDeviceWires(wires); | ||
|
|
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.
Any reason for switching the order of these steps?
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.
That's because the getMeasurements() is within the GenerateSamples. The getMeasurements() will compact the state vector to get the measurement object, that means if the getDeviceWires happens before GenerateSamples may not get the correct device wires.
std::vector<size_t> LightningSimulator::GenerateSamples(size_t shots) {
auto m = getMeasurements();
// ... rest of code ...
| std::unordered_set<DeviceQubitID> new_free_qubits; | ||
| for (auto old_device_id : this->free_device_qubits) { | ||
| auto it = old_to_new_mapping.find(old_device_id); | ||
| if (it != old_to_new_mapping.end()) { | ||
| new_free_qubits.insert(it->second); | ||
| } else { | ||
| new_free_qubits.insert(old_device_id); | ||
| } | ||
| } | ||
| this->free_device_qubits = std::move(new_free_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.
How often do we expect this section to be run? Will it be expensive to create and free the free_device_qubits set each time this is run as we currently do?
| for (size_t old_idx = 0; old_idx < old_data.size(); old_idx++) { | ||
| size_t new_idx = 0; | ||
| for (size_t i = 0; i < num_qubits_after; i++) { | ||
| size_t old_wire = wire_id_pairs[i].first; | ||
| size_t old_bit_pos = old_num_qubits - 1 - old_wire; | ||
| size_t new_bit_pos = num_qubits_after - 1 - i; | ||
|
|
||
| if ((old_idx >> old_bit_pos) & 1) { | ||
| new_idx |= (1UL << new_bit_pos); | ||
| } | ||
| } | ||
|
|
||
| new_data[new_idx] += old_data[old_idx]; | ||
| } |
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.
This outer loop goes over the entire statevector, we probably want to use parallelism to prevent this from being a very expensive operation. Unsure if it's worth doing here or fixing in a follow-up PR. @maliasadi
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]]