-
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
Adds Hamiltonian Queueing and Transform to Compute Hamiltonian Expectation Value #1142
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1142 +/- ##
==========================================
+ Coverage 98.16% 98.25% +0.09%
==========================================
Files 145 146 +1
Lines 11099 11129 +30
==========================================
+ Hits 10895 10935 +40
+ Misses 204 194 -10
Continue to review full report at Codecov.
|
Thanks @Lucaman99! Let me know when this is ready for review :) |
Hey @ixfoduap @josh146, I think this PR is pretty much ready for review! The only thing I haven't done yet is the tests for the actual queuing of the Hamiltonians, as this might change (since the queuing is currently causing some minor issues, as are described above). Other than that, the functionality is done, and the tests/docstring for the |
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.
Hey @Lucaman99, this is great! I really like how you have set up written, and organized this PR.
A couple of comments and suggestions while going through it:
-
A big part of the work here was making sure that Hamiltonians queue correctly and are accepted by
expval
. However, I noticed that thehamiltonian_expval
function itself shares a lot of logic withmeasurement_grouping
. We should be able to take advantage of this, by simply calling it:def hamiltonian_expval(tape): H = tape.measurements[0].obs if not isinstance(H, qml.Hamiltonian) or len(tape.measurements) > 1: raise ValueError( "Passed tape must end in `qml.expval(H)`, where H is of type `qml.Hamiltonian`" ) H.simplify() return qml.transforms.measurement_grouping(tape, H.ops, H.coeffs)
I tested this out locally, and it works really well! I suggest making this change here.
-
Another important aspect we need to take into account is differentiability. That is, we should always have tests that check for the gradient, and not just the result, of a transform!
Currently, the
hamiltonian_expval
is not differentiable, since it uses standard NumPy. However, themeasurement_grouping
transform usesqml.math
which does allow for differentiability. With the change in the point above, differentiating now works:import pennylane as qml from pennylane import numpy as np from pennylane.interfaces.autograd import AutogradInterface dev = qml.device("default.qubit", wires=3) with qml.tape.JacobianTape() as tape: for i in range(2): qml.RX(np.array(0, requires_grad=True), wires=0) qml.RX(np.array(0, requires_grad=True), wires=1) qml.RX(np.array(0, requires_grad=True), wires=2) qml.CNOT(wires=[0, 1]) qml.CNOT(wires=[1, 2]) qml.CNOT(wires=[2, 0]) H = -0.2 * qml.PauliX(1) + 0.5 * qml.PauliZ(1) @ qml.PauliY(2) + qml.PauliZ(0) qml.expval(H) AutogradInterface.apply(tape) def cost(x): tape.set_parameters(x) tapes, fn = qml.transforms.hamiltonian_expval(tape) res = [t.execute(dev) for t in tapes] return fn(res)
where
>>> x = np.array([0.1, 0.67, 0.3, 0.4, -0.5, 0.7], requires_grad=True) >>> print(cost(x)) 0.42294409781940345 >>> print(qml.grad(cost)(x)) [ 9.68883500e-02 -2.90832724e-01 -1.04448033e-01 -1.94289029e-09 3.50307411e-01 -3.41123470e-01]
We should also verify it works for the other interfaces, e.g., TF:
import pennylane as qml import tensorflow as tf from pennylane.interfaces.tf import TFInterface dev = qml.device("default.qubit", wires=3) H = -0.2 * qml.PauliX(1) + 0.5 * qml.PauliZ(1) @ qml.PauliY(2) + qml.PauliZ(0) x = tf.Variable([[0.1, 0.67, 0.3], [0.4, -0.5, 0.7]], dtype=tf.float64) with tf.GradientTape() as gtape: with qml.tape.JacobianTape() as tape: for i in range(2): qml.RX(x[i, 0], wires=0) qml.RX(x[i, 1], wires=1) qml.RX(x[i, 2], wires=2) qml.CNOT(wires=[0, 1]) qml.CNOT(wires=[1, 2]) qml.CNOT(wires=[2, 0]) qml.expval(H) TFInterface.apply(tape) tapes, fn = qml.transforms.hamiltonian_expval(tape) res = fn([t.execute(dev) for t in tapes])
which gives
>>> print(res) tf.Tensor(0.42294409781940345, shape=(), dtype=float64) >>> grad = gtape.gradient(res, x) >>> print(grad) [[ 9.68883500e-02 -2.90832724e-01 -1.04448033e-01] [-1.94289029e-09 3.50307411e-01 -3.41123470e-01]], shape=(2, 3), dtype=float64)
So once you are using
measurement_grouping
insidehamiltonian_expval
, you should be able to write tests for the gradient :) -
Finally, with respect to the queuing issue -- I have to admit, I wasn't able to track it down. My gut feeling is that one of the manipulations that occurs during arithmetic (e.g., so
Observable.__mul__
orTensor.__mul__
must not be queuing correctly? Do you think you might be able to track down the issue here?
One final, tangential thought for the future: currently, the Hamiltonian
class always casts coeffs to a list, which breaks differentiability! We should try and make the Hamiltonian class differentiable, there isn't a reason why we couldn't have coefficients that are tensorflow variables :)
|
||
class TestHamiltonianExpval: | ||
"""Tests for the hamiltonian_expval transform""" | ||
|
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 slightly more annoying, it would be very beneficial to have some unit tests to ensure that the tape is constructed properly, by explicitly checking tape.operations
and tape.measurements
!
@Lucaman99 a sudden thought - what happens if you try and draw a tape that has |
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 @Lucaman99! Just left a few questions, let me know when this is ready for a final review.
pennylane/transforms/__init__.py
Outdated
@@ -18,3 +18,4 @@ | |||
from .draw import draw | |||
from .measurement_grouping import measurement_grouping | |||
from .metric_tensor import metric_tensor | |||
from .hamiltonian_expval import hamiltonian_expval |
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.
Will hamiltonian_expval
be user facing? If so, we should add to the main qml.__init__
like with the other transforms above.
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.
Good question. Maybe we can re-evaluate once the QNode part of the feature is live? Easier to decide when all the parts are in.
if not isinstance(hamiltonian, qml.Hamiltonian) or len(tape.measurements) > 1: | ||
raise ValueError( | ||
"Passed tape must end in `qml.expval(H)`, where H is of type `qml.Hamiltonian`" | ||
) |
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 raise message doesn't cover the case of multiple measurements (i.e., len(tape.measurements) > 1
). Maybe that can be made into a separate if
statement.
dot_products = [ | ||
qml.math.dot(qml.math.convert_like(c, r), r) for c, r in zip(coeffs_groupings, res) | ||
] | ||
return qml.math.sum(qml.math.stack(dot_products)) |
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.
🤔 Maybe good to leave a comment on the PR for subtle changes like this. Why we need to edit measurement_grouping
?
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 odd, I don't recall making this change. Maybe something weird happened while merging, I'll fix it.
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 no, this was me while I was testing your PR @Lucaman99!
This actually fixed a bug, can we keep this in?
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 were two issues here:
-
the Hamiltonian class casts the
coeffs
to a list, which is a bit annoying. It requires casting it back to a tensor by doingqml.math.convert_like(c, r), r)
. -
Secondly, the resulting list of dot products needs to be stacked before summation to support TensorFlow, which does not like summing a list of tensors.
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 the end this transform is so simple! (Once you know what to do!)
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 good from my end @Lucaman99! Just a couple of small changes before it is ready for merging.
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@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.
💯
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
🎉 |
IMPORTANT NOTE
Currently, there is a small issue with this PR: when initializing a Hamiltonian using the arithmetic operations between Observables/Tensors/other Hamiltonians, the queue gets slightly confused, and Observables with no owners get added to the queue (for reasons I have yet to fully understand).
As a result, I have had to comment out an error to get things to work properly. Funnily enough, if we didn't have this error implemented, then everything would work fine (as Observables without owners that are added to the queue have no effect on the results of running the circuit). Suggestions as to how we can deal with this issue are much appreciated!
Context:
Description of the Change:
qml.Hamiltonian
instances to be queued in the quantum tape.For instance, we can define the following tape:
Then, we can map it to multiple tapes, corresponding to different terms of the Hamiltonian
Finally, we can execute each of the tapes and pass them through a post-processing function to return the expectation value:
Benefits:
qml.ExpvalCost
, which does something similar (but instead splits the QNode into a QNodeCollection, allowing for parallelization)Possible Drawbacks:
None
Related GitHub Issues: