-
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
[SHOTS] Allows a list of shots to be specified #1103
Conversation
Nice! One thought, we cannot assume that other devices inheriting from |
Codecov Report
@@ Coverage Diff @@
## master #1103 +/- ##
=======================================
Coverage 97.73% 97.74%
=======================================
Files 154 154
Lines 11683 11726 +43
=======================================
+ Hits 11418 11461 +43
Misses 265 265
Continue to review full report at Codecov.
|
Is |
# count the basis state occurrences, and construct the probability vector | ||
for b, idx in enumerate(indices): | ||
basis_states, counts = np.unique(idx, return_counts=True) | ||
prob[basis_states, b] = counts / bin_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.
I wonder if this can be vectorized, rather than a for loop? Perhaps it's also possible to do away with the if/else, and have a single block of logic for any bin 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.
I'm not certain it can easily be vectorized just because the output shape of np.unique
is different each loop.
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.
😢
For devices that don't know about |
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.
I'm not feeling super keen on this right now. It feels like we're adding a ton of complexity for what amounts to a for loop in user code.
number of shots and the shot vector. | ||
|
||
Args: | ||
shot_list (Sequence[int, tuple[int]]): sequence of non-negative shot integers |
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.
Why is tuple[int] included in sequence?
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.
At the back of my mind, I wanted to allow (advanced) users and other parts of the codebase to pass shots=[(1, 1e9)]
, rather than having to do shots=[1] * 1000000000
, which will create a large list in memory for absolutely no reason
# count the basis state occurrences, and construct the probability vector | ||
for b, idx in enumerate(indices): | ||
basis_states, counts = np.unique(idx, return_counts=True) | ||
prob[basis_states, b] = counts / bin_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.
I'm not certain it can easily be vectorized just because the output shape of np.unique
is different each loop.
pennylane/_device.py
Outdated
total_shots = shot_list[0] * len(shot_list) | ||
else: | ||
# Iterate through the shots, and group consecutive identical shots | ||
split_at_repeated = np.split(shot_list, np.where(np.diff(shot_list) != 0)[0] + 1) |
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.
I'm having a hard time parsing this line.
So the first np.diff(shot_list) != 0
is a boolean array with the first indicies where the shot value changes marked as True, and np.where(...)
returns tuple of an array of the indices of these first values. Is that correct?
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.
pennylane/_device.py
Outdated
if len(set(shot_list)) == 1: | ||
# All shots are identical; represent the shot vector | ||
# in a sparse format. | ||
shot_vector = [(shot_list[0], len(shot_list))] | ||
total_shots = shot_list[0] * len(shot_list) |
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 chunk isn't needed as the code below should correctly handle this case
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.
True, I added this after for two reasons:
- I think
shots=[n] * N
is probably the most common use-case - In the case of
shots=[n] * N
, this is faster than the second branch.
But yes, the second branch should handle both cases
Co-authored-by: Chase Roberts <chase@xanadu.ai>
Unfortunately, a for loop in the user code (while giving the same results) will generate separate jobs that may be held up in a remote queue. The idea here is to 'batch' shots on a single QNode eval. Shot adaptive quantum optimizers typically need to generate many 1-shot expectation values, which is currently not possible to implement in PennyLane without submitting many single-shot jobs. Note also that the loop is not over single shots, but batches of shots which are vectorized. I imagine arguments of the form |
@Thenerdstation have standardized the format to a namedtuple, and made every element a tuple - it cleans it up a lot, and helps remove a lot of redundant lines. |
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 feels a lot cleaner thank you!
Approval for code structure and readability, but I leave it to nathan and maria to decide if we want to break the existing plugins for this.
Np!
I expect #1079 to pre-break all plugins, so took a little liberty here 😆 |
That's fair |
expectation values of observables | ||
shots (int, list[int]): Number of circuit evaluations/random samples used to estimate | ||
expectation values of observables. If a list of integers is passed, the circuit | ||
evaluations are batched over the list of shots. |
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.
I think this will need some more explanation? For example, what is the rule for circuit outputs? If the device is analytic and qml.probs(), qml.sample()
is returned, will only the second return value be batched? But will both be batched if the device is non-analytic?
I know that this will somewhat change by shots refactor II, but good to be precise?
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.
Yes, good point. At the moment, anything that relies on samples will get batched if the shots are batched. So that includes samples always, and expval/var/probs in non-analytic mode.
This will definitely be more intuitive after II, because the presence of batched shots will always correspond to batched output.
qml.RX(x, wires=0) | ||
qml.RY(y, wires=0) | ||
qml.CNOT(wires=[0, 1]) | ||
return qml.probs(wires=0), qml.probs(wires=1) |
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.
I am actually not sure now, but do we allow for mixed-measurement-process outputs like probs(), expval()
? If so, isn't that an important edge case?
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 undefined behaviour at the moment 🤔 This results in a ragged array, and I purposely didn't try to solve it due to an upcoming change in NumPy that will make it really hard to work with object arrays
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 one @josh!
Just a few questions before approving, but it looks good to me.
qml.device("default.qubit", wires=2, analytic=False, shots=0.5) | ||
|
||
with pytest.raises(ValueError, match="Unknown shot sequence"): | ||
qml.device("default.qubit", wires=2, analytic=False, shots=["a", "b", "c"]) |
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 need a test for the per-qnode-call way of setting shots?
Context: Expands the functionality of the QNode, by allowing measuring statistics to be course grained over shots in a single evaluation.
Description of the changes:
Consider
When QNodes are executed on this device, a single execution of 1015 shots will be submitted.
However, three sets of measurement statistics will be returned; using the first 5 shots,
second set of 10 shots, and final 1000 shots, separately.
For example:
Executing this, we will get an output of size
(3, 2)
:>>> circuit(0.5) [[0.33333333 1. ] [0.2 1. ] [0.012 0.868 ]]
The output remains fully differentiable.
Benefits:
Drawbacks: Breaks all plugins that depend on
QubitDevice
, due to two new keyword arguments inexpval
,var
, andsample
.