-
Notifications
You must be signed in to change notification settings - Fork 575
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
Faster QubitDevice.marginal_prob #799
Conversation
Codecov Report
@@ Coverage Diff @@
## master #799 +/- ##
=======================================
Coverage 91.09% 91.10%
=======================================
Files 130 130
Lines 8701 8709 +8
=======================================
+ Hits 7926 7934 +8
Misses 775 775
Continue to review full report at Codecov.
|
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 is a huge improvement @antalszava! Great work 🙂
pennylane/_qubit_device.py
Outdated
states_base_ten = np.arange(2 ** num_wires, dtype=np.int32) | ||
basis_states = self.states_to_binary(states_base_ten, num_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.
Can this be made less memory intensive by having states_base_ten
be a generator (so that you don't have to store all basis states in memory at once?)
I'm not sure this idea would work though, since then you might not be able to perform the bitwise shift <<
🙂
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.
Have looked into this. As we'd like to eventually use basis_states
to compute perm
and permute the probabilities, we'd need to have all the basis states in a single array at one time.
One thing I found was that we can use a 32-bit integer for casting in states_to_binary
, that helped with memory to some extent (turned to 32 bits from 64).
pennylane/_qubit_device.py
Outdated
# therefore this approach is used only for fewer wires. | ||
# For smaller number of wires speed is comparable to the next | ||
# approach, hence we resort to that one for testing purposes | ||
states_base_ten = np.arange(2 ** num_wires, dtype=np.int32) |
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.
If you use np.uint32
, do we make it to 32 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.
Thanks! Yeah, could increase it so that we can have 31 qubits (overflow at 32)
Co-authored-by: Josh Izaac <josh146@gmail.com>
…nnylane into faster_marginal_prob
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
…nnylane into faster_marginal_prob
…nnylane into faster_marginal_prob
…nnylane into faster_marginal_prob
…nnylane into faster_marginal_prob
Thanks @josh146 @trbromley for the comments! Addressed them. (Also found a cool memory profiler, |
Context:
While looking at improvements for the
QubitDevice
class, it was found, that certain internal functions for computing statistics might not scale very well with larger numbers of qubits. One method, in particular, themarginal_prob
method seemed to have performed poorly with an increasing number of wires.Running a benchmark for 22 qubits and extracting probabilities showed the following results:
This and further results for smaller systems showed that considerable time is taken for conversion to
![image](https://user-images.githubusercontent.com/24476053/93164400-5f00fd00-f6e7-11ea-8f8a-adf32595c81d.png)
numpy.array
:A
numpy.array
is being created in the following expression as part ofmarginal_probs
:Description of the Change:
basis_states
by using thestates_to_binary
method.np.fromiter
anditertools.chain
Benefits:
marginal_probs
and sub-methods to the overall time taken for the benchmark on extracting probabilities (>205.138 seconds -> ~35 seconds):states_to_binary
method is not ideal to be used (>30 qubits):Original:
Possible Drawbacks:
The
states_to_binary
method creates basis states faster (for larger systems at times over x25 times faster) than the approachusing
itertools
, at the expense of using slightly more memory when usingdtype=int64
. Hence we constraint thedtype
of the array to representing unsigned integers on 32 bits (uint32
).Due to this constraint, an overflow occurs for 32 or more wires:
Therefore this approach is used only for fewer wires.
For a smaller number of wires, speed is comparable to the improved original approach, hence we resort to that one for testing purposes
Related GitHub Issues:
N/A