-
Notifications
You must be signed in to change notification settings - Fork 603
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
[Archived] Raise an error when we return sample()
or counts()
with observables that are not diagonal!
#2924
Conversation
[sc-23709] |
Hello. You may have forgotten to update the changelog!
|
@antalszava and @rmoyard, It seems some of the tests for the new return types are failing due to this bug fix. It seems that in these tests you are actually measuring things that "can't" be measured together. Let me know what you think I should do to account for this? I can also just remove this update from the new execute method and you can add it in the respective PR specifically. |
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 don't think that's the right way to go here. We have a system to support non commutative observables by batching the tapes. Normally it should split the tapes between the non-commuting part and creates multiple tapes. For sample it is not supported at the moment. We have the following check in split_non_commuting.py
if qml.measurements.Sample in return_types or qml.measurements.Probability in return_types:
raise NotImplementedError(
"When non-commuting observables are used, only `qml.expval` and `qml.var` are supported."
)
It is called in batch_transform
in _device.py
. Imo it would make more sense to update split_commuting
and batch_transform
in order to support Sample
and Counts
. I think it implies changing the tapes._obs_sharing_wires
as well.
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 change overall looks good, there are some subtleties to update.
It seems some of the tests for the new return types are failing due to this bug fix. It seems that in these tests you are actually measuring things that "can't" be measured together. Let me know what you think I should do to account for this?
If the tests are testing cases that should not be tested, then we should either remake them (if the new version adds value) or just remove them).
pennylane/_qubit_device.py
Outdated
if o.return_type in (qml.measurements.Sample, qml.measurements.Counts): | ||
sampled_obs.append(o) | ||
|
||
raw_sampled_ops = any(o.obs is None for o in sampled_obs) |
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.
Could we get this information from the previous for
loop via if o.return_type in (qml.measurements.Sample, qml.measurements.Counts) and o.obs is None:
? Might also be good to stop at the first occurrence.
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.
Great catch!
@rmoyard I agree! This is what I mentioned in the meeting that the better / long term solution is to support Sample and Counts in We discussed in the short term to raise an error as without the error users would be able to execute this and the wrong samples / counts with no indication that it's wrong. I initially thought about raising the error at the tape level (in |
Co-authored-by: antalszava <antalszava@gmail.com>
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.
As you mentioned right now we are allowing non-commuting measurements when using qml.sample()
and qml.counts()
without any observable. However when using an observable this is not allowed:
import pennylane as qml
dev = qml.device("default.qubit", wires=4, shots=1000)
@qml.qnode(dev)
def circuit():
return qml.expval(qml.PauliX(0)), qml.sample(qml.PauliZ(0))
print(circuit())
Raises:
QuantumFunctionError: Only observables that are qubit-wise commuting Pauli words can be returned on the same wire.
My question is, could we re-use this Error for the non-observable case?
Using qml.counts()
and qml.sample()
with wires=None
and obs=None
should be equivalent to sampling all wires on the basis state, right? But this is not represented in the MeasurementProcess
class, given that wires
and obs
are still None:
>>> meas = qml.counts()
>>> meas.obs
>>> meas.wires
<Wires = []>
Consequently, these measurements are omitted in the _update_observables
method of the QuantumTape
class, and tape._obs_sharing_wires
is kept empty
pennylane/pennylane/tape/tape.py
Line 517 in 2582fcf
obs_wires = [wire for m in self.measurements for wire in m.wires if m.obs is not None] |
and python skips the check for commuting observables:
pennylane/pennylane/_device.py
Line 654 in 1f149fc
if ops_not_supported or obs_on_same_wire: |
pennylane/pennylane/tape/tape.py
Line 159 in 436d85c
raise qml.QuantumFunctionError( |
All in all, wouldn't it be easier to (somehow) set wires=all_wires
and obs=qml.PauliZ
when using qml.counts()
and qml.sample
? Then we would obtain the commutativity check for free.
Maybe this is not in scope, but I realised that running the code above with qml.counts
doesn't raise an error. I will try to figure out why.
Edit: Found the source of the bug: when using qml.counts
, split_non_commuting
was called and changed qml.counts
to qml.expval
. To solve this an if qml.measurements.Counts not in return_types
check must be added here:
pennylane/pennylane/_device.py
Line 736 in 2582fcf
and not qml.measurements.Probability in return_types |
I added this in PR #2928.
@rmoyard these checks are already done before calling this function: pennylane/pennylane/_device.py Line 736 in 2582fcf
Do you think we could remove them? |
This is also a good idea @AlbertMitjans. One issue would be that when we provide an observable (say PauliZ), the raw samples (which will be 0s, or 1s) get transformed to be samples of the eigenvalues of the observable (-1s and 1s in this case). So doing something like what you suggested on the level of |
What about just setting the pennylane/pennylane/tape/tape.py Line 517 in 2582fcf
I am insisting on this because I find it inconsistent that when using |
sample()
or counts()
with observables that are not diagonal! sample()
or counts()
with observables that are not diagonal!
Context:
sample and counts are measurement processes which admit a special syntax:
sample()
,counts()
. We interpret this syntax to mean "return computational basis samples or counts". However, this measurement is not commutative with other measurements which are not diagonal in the computational basis. Thus they are not allowed to be performed simultaneously.We now raise an error when the user tries to measure these simultaneously with other non-commuting measurements.
Description of the Change:
Add check in
execute()
to determine if there are any diagonalizing gates and if we are also asking for computational basis samples.Related GitHub Issues:
#2863