-
Notifications
You must be signed in to change notification settings - Fork 28
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
Adding support for multi-qubit observable estimation #42
Conversation
…odifying docstring for tests
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.
Looks great @msohaibalam! Excited for this. One thing that I've noticed is that the old operator estimation API doesn't support parametric compilation, whereas the new one developed by @karalekas does. @antalszava takes this into account in one of the previous PRs, by making operator estimation and parametric compilation mutually exclusive, but it would be great to port to the new API when stable/ready.
pennylane_forest/qpu.py
Outdated
# estimation | ||
if isinstance(observable, Tensor): | ||
observable = observable.obs[0] | ||
wires = observable.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.
👍 I think I saw @antalszava mention this elsewhere, but we have just made an update to the logic in PennyLane core. If the user defines a tensor of a single observable Tensor(qml.PauliX(0))
, the QNode will now smartly simply pass the observable to the device.
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.
Oh yeah, this is just code from @antalszava's PR which I decided to build off instead of master. I'll merge in the new changes.
pennylane_forest/qpu.py
Outdated
# Multi-qubit observable | ||
if len(wires) > 1: | ||
# In this case, the observable must be a Tensor product | ||
assert isinstance(observable, Tensor), "Multi-qubit observable is not in Tensor format" |
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.
Ah, this might not be the case! We allow qml.Hermitian(A, wires=w)
, where A.shape = [2**w, 2**w]
. So the if statement should probably be
if len(wires) > 1 and isinstance(observable, Tensor):
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.
Got it! It sounds like then that the check for observable.name == "Hermitian"
should handle both the single-qubit and multi-qubit observable cases then. I'll make the change.
pennylane_forest/qpu.py
Outdated
|
||
# All observables are rotated to be measured in the Z-basis, so we just need to | ||
# check which wires exist in the observable, map them to physical qubits, and measure | ||
# the product of PauliZ operators on those 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.
Nice!
symmetrize_readout=self.symmetrize_readout, | ||
calibrate_readout=self.calibrate_readout, | ||
) | ||
) |
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.
Something that @antalszava pointed out is that measure_observables
will implicitly execute the circuit on the QPU. However, QVMDevice.generate_samples()
has already done one execution, making it redundant. This could be solved by simply overwriting generate_samples()
here to take into account operator estimation, or perhaps by our eventual goal of merging QVM and QPU 😆
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.
Not sure I follow this point. Is the idea QVMDevice.generate_samples()
being called before/while this particular call to measure_observables
? (and if that's the case, it's not too clear to me where exactly this might be happening)
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, one missing piece here is that we have moved to a new API in PennyLane and this affects the plugins as well.
Following up on the previous #38, now ForestDevice inherits from QubitDevice. The PennyLane type of flow of logic involves evaluating a quantum circuit by calling on the execute method of the device (such is QPUDevice).
Each instance of the ForestDevice (now QPUDevice in our case) will currently inherit the same execute method as seen here (which is not redefined in any of the subclasses).
As per the logic in QubitDevice.execute
, currently generate_samples
is called, as the QPUDevice
does not support analytic evaluation (it being a hardware). After this, statistics are extracted and therefore eventually we call on QPUDevice.expval
which usually corresponds to QubitDevice.expval
.
Now in our case, QPUDevice
inherited the QVMDevice.generate_samples
method, which makes the call on the QuantumComputer.run
method. It will in the expval
method, however, call on measure_observables
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.
Thanks for the detailed explanation @antalszava! It seems to me this won't be the last time generate_samples
and measure_observables
will conflict ;) , mostly because they're both trying to generate samples!
My gut feeling is also that even if we do merge the QVM and QPU classes, we'll still be faced with the choice of either over-riding generate_samples
to use measure_observables
, or port over all the logic of error mitigation over to generate_samples
(and/or its helper functions). I'm not sure how I can over-ride it here though, would appreciate input on that!
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 sure how I can over-ride it here though, would appreciate input on that!
I think it's a good question, and not entirely trivial --- I say we merge this as is, and make a new issue to determine how to avoid this conflict!
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 sounds good to me. I've created a new issue here: #45.
tests/test_qpu.py
Outdated
res = circuit_tensor(p) | ||
exp = circuit_obs(p) | ||
|
||
assert np.allclose(res, exp, atol=2e-2) |
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.
Awesome!
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.
Borrowed code from @antalszava's PR :)
tests/test_qvm.py
Outdated
assert dev.circuit_hash in dev._compiled_program_dict | ||
assert len(dev._compiled_program_dict.items()) == 1 | ||
|
||
@flaky(max_runs=10, min_passes=1) | ||
@flaky(max_runs=5, min_passes=3) |
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.
While not best practice for unittests, we've found flaky really nice for stochastic quantum integration tests 😆
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.
Seems like a nice tool!
Co-Authored-By: Josh Izaac <[email protected]>
Co-Authored-By: Josh Izaac <[email protected]>
Thanks for the feedback, @josh146 and @antalszava! I'm done making changes and updates, so please give this another look when you can. Re: operator_estimation not handling parametric compilation, yes that is unfortunately a feature currently lacking, and one of the aims of the new API (once it's ready) is to include precisely this functionality. |
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 looks great @msohaibalam, thanks for the help getting it over the line! I've left a few comments, but nothing major.
pennylane_forest/qpu.py
Outdated
# API has been executed. This new API provides compatibility between parametric | ||
# compilation and operator estimation. | ||
warnings.warn("Parametric compilation is currently not supported with operator" | ||
"estimation. Operator estimation is being turned off.") |
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 come this is showing up as a change, has the location been moved?
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 changed this to have the same format as on the master branch.
from pyquil.quil import Pragma, Program | ||
from pyquil.operator_estimation import (Experiment, ExperimentSetting, | ||
TensorProductState, group_experiments, | ||
measure_observables) |
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 to have the updated import locations!
pennylane_forest/qpu.py
Outdated
if observable.name == "Hermitian": | ||
# <H> = \sum_i w_i p_i | ||
Hkey = tuple(par[0].flatten().tolist()) | ||
w = self._eigs[Hkey]["eigval"] | ||
return w[0] * p0 + w[1] * p1 |
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 might not be needed, as the call to super().expval(observable)
already takes into account 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.
Great! Removing this code block.
pennylane_forest/qpu.py
Outdated
return w[0] * p0 + w[1] * p1 | ||
|
||
# `measure_observables` called only when parametric compilation is turned off | ||
elif not self.parametric_compilation: |
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.
In which case, this can simply become if not self.parametric_compilation:
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.
Updated.
pauli_obs = sI() | ||
for wire in observable.wires: | ||
qubit = wire[0] | ||
pauli_obs *= sZ(self.wiring[qubit]) |
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.
Awesome!
symmetrize_readout=self.symmetrize_readout, | ||
calibrate_readout=self.calibrate_readout, | ||
) | ||
) |
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 sure how I can over-ride it here though, would appreciate input on that!
I think it's a good question, and not entirely trivial --- I say we merge this as is, and make a new issue to determine how to avoid this conflict!
…out parametric compilation
Extends
qpu.expval
to handle multi-qubit observable estimation.Handles the scenarios where (a) multi-qubit observable come in the
Tensor
format, and (b) pre-rotations are appropriately applied to all qubits so that we only need to measure the PauliZ operator.Cleans up and consolidates the logic to remove code repetition between single and multi-qubit observable estimation.
A few things to note:
readout_error=[p00, p11]
for simplicity. If we need the ability to define different readout errors on distinct qubits, we might choose to promotereadout_error
to be a list of lists in the multi-qubit case and handle that scenario separately.