-
Notifications
You must be signed in to change notification settings - Fork 42
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
Serialize obs with pauli reps into hamiltonians #424
Conversation
Codecov Report
@@ Coverage Diff @@
## master #424 +/- ##
=======================================
Coverage 99.82% 99.82%
=======================================
Files 50 50
Lines 4635 4654 +19
=======================================
+ Hits 4627 4646 +19
Misses 8 8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Thank you, @timmysilv. Nice job here!
Could you please add some benchmarks to this PR showing how these changes impact performance?
Also, you need to update the changelog.
[sc-22425] |
I generated random Pauli sentences for 1-8 wires with a reasonable number of terms in each, then converted each into an old-style Hamiltonian, as well as a new-style arithmetic op (with a pauli rep). I also downloaded Hamiltonians for H6, H2O and BH3 (12, 14, 16 qubits, respectively) and stuck those into my benchmark data. I ran The two observable types are at near-parity, with some solid boosts using the new operator type! I'll note that occasionally, I saw the old-school observable winning by microseconds in very small systems (1-2 qubits, a handful of terms) but I think that isn't very significant. Please note that the data shown below is plotted with a log scale, meaning that the new observable type does significantly better in larger systems. Also, the performance boost is likely due to pre-processing. Hamiltonians will use hamiltonian_expand(group=False) to split tapes, whereas Sum observables will use sum_expand. Given that the observables should be identical other than type/python representation, this makes the most sense. Example with my code (16-qubit Hamiltonian): >>> ham_circuit = qml.tape.QuantumScript(measurements=[qml.expval(hams[-1])])
>>> op_circuit = qml.tape.QuantumScript(measurements=[qml.expval(ops[-1])])
>>> ham_tapes, _ = qml.transforms.hamiltonian_expand(ham_circuit, group=False)
>>> op_tapes, _ = qml.transforms.sum_expand(op_circuit)
>>> len(ham_tapes), len(op_tapes)
(771, 433) PS: I converted my notebook to a python file for your viewing (if you're curious about what I did). I can't upload .py or .ipynb files to a PR, so I made a gist! |
Hi @timmysilv. Thank you for the benchmarks! I'm happy to see the performance improvements. |
This needs more work. I just ran some validation and it makes an actual difference with results. I'm not sure why, but this big slow molecule demonstrates it: import pennylane as qml
from timeit import default_timer as timer
ham = qml.data.load("qchem", molname="BH3", basis="STO-3G", bondlength=0.86)[0].hamiltonian
op = qml.pauli.pauli_sentence(ham).operation()
dev = qml.device("lightning.qubit", op.wires)
@qml.qnode(dev)
def c_ham():
return qml.expval(ham)
@qml.qnode(dev)
def c_op():
return qml.expval(op) expectation with hamiltonian is 10.295583513349388, but with op it's 9.790384688804833. This shouldn't be related to the Identity change because that isn't used in non-adjoint expectations. How to convince myself that the observables are indeed equivalent... will try on Monday. EDIT: it's related to how they are split: >>> dev.expval(ham), dev.expval(op)
(10.295583513349388, 10.295583513349388) |
Hmm. You are right. There is something weird here. I hope this can help you map out the problem. |
Thanks @timmysilv |
I was wondering the same thing, so fortunately I have the answer for you! I just got Utkarsh to give me a 20-qubit, 2951-term Hamiltonian for Li2 and they remain neck and neck even up there (ran a few times locally, they seem pretty much equal. Sums win by up to 5%). I also tried with a 24-qubit, 30k-term Hamiltonian but my poor old MacBook Air just couldn't do it, so you'd need to ask a more powerful computer to confirm what happens beyond 20. |
PRs (and follow-up work) needed for this to be ready:
(My benchmarking was done with all these changes present in my local environment) |
Thanks @timmysilv . I think given the agreement at 20, the bottleneck is likely no longer the routines modified here, and may instead be other parts of the pipeline. I'd say that's fine for now. |
re: the above conversation, I've changed QubitDevice in PennyLane to provide a hook called |
this is ready for review again! my latest benchmarks show the old and new operators taking the same amount of time |
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.
Nothing more to add from my side.
Thank you for that!
Co-authored-by: Amintor Dusko <[email protected]>
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 @timmysilv
I have some questions and some suggested updates.
Also, since this work will likely need to be ported to L-GPU and L-Kokkos also, it may be worth planning that out too so that the packages stay in-line.
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'll prepare the changes for kokkos and gpu
Co-authored-by: Lee James O'Riordan <[email protected]>
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 all the work @timmysilv
Happy to approve.
Context:
Arithmetic operators are the latest craze in PennyLane, and lightning should support them natively as well! If an observable (that isn't a Tensor or Hamiltonian) has a representation in the Pauli basis, it will be saved on the Operator. Those representations can easily be converted into what is passed to the existing C++ bindings for PennyLane Hamiltonians.
Description of the Change:
PauliSentence
objects intoHamiltonianCxx
lightning classes_pauli_rep
assigned to it_obs_has_kernel
did some consideration for Hamiltonians and Tensors that wasn't used because it was only called on things we knew were neither of those. The new flow should be simpler and more readable, and it lends itself better to op-arithmetic supportBenefits:
qml.prod
and other arithmetic operators were previously defaulting toqml.matrix(op)
, and that is not efficient. With this PR, they are being serialized (viaHamiltonian
for now) and computed with that C++ kernel. This change supports all operators with a_pauli_rep
attribute.Possible Drawbacks:
This won't cover all cases. For example, consider the following:
all_pauli
anduses_hadamard
are the same, and are valid observables, but only the former will be serialized. This is most unfortunate because there are kernels for Pauli ops and Hadamard, but I don't cover that with this PR.Note that as a sideways fix to this, I propose a Pauli representation for Hadamard in another PR (linked below).EDIT: this is a larger problem with op-arithmetic (should also considerqml.Hermitian
) and needs more work on the PL side.