-
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
Add a modified simplify function for building Hamiltonians #2103
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #2103 +/- ##
=======================================
Coverage 99.20% 99.20%
=======================================
Files 230 230
Lines 17717 17718 +1
=======================================
+ Hits 17576 17577 +1
Misses 141 141
Continue to review full report at Codecov.
|
|
||
return qml.Hamiltonian(coeffs, ops) | ||
|
||
|
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.
It would be great to reproduce this pattern in the Hamiltonian
class. At the moment, H.simplify()
is a method that manipulates the internal data of H
, which is dangerous. We'd like it to return a new operator. Another option is qml.simplify(H)
, which could be cool because other ops may be simplified in some way too.
I don't think this relates to this PR though, which is just about building Hamiltonians in a special way, right?
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 PR is literally adding a function that simplifies any input Hamiltonian. As far as I can tell, it should be possible to reproduce in the Hamiltonian class, or as you suggest, having qml.simplify(H)
. I really like this latter suggestion actually
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.
Hi @mariaschuld, @ixfoduap and @josh146. Here are my thoughts on the simplify function.
This is a general functions that does not necessary belong to qml.hf
. However, the way it is written currently is based on one important point: convert Pauli words to strings and manipulate the strings with python. For example, instead of looping over the words and using compare
, one can simply use
if o in ['XYZ', ...]
However, I am not sure we want to manipulate Hamiltonians/operators by turning them to strings. Another philosophy could be generating the binary representation of the words and Hamiltonians and manipulate those objects numerically to, for example, compare or multiply them. I think we need to decide first what approach we want to follow for operator manipulation/arithmetic in general. This is the only reason that I hided _simplify
inside hf.hamiltonian
for now.
So, I want your thoughts on these options:
- Convert
hf.hamiltonian._simplify
toqml.simplify(H)
. - Wait until we have an ADR on how we really want to efficiently work with operators/Hamiltonian in PL and then create
qml.simplify(H)
accordingly. - Change
hf.hamiltonian._simplify
tohf.hamiltonian.simplify
.
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.
@soranjh I think these are very valid points, still a lot needs to be figured out in terms of how to best implement comparison/simplification long-term.
Having said that, it's not quite clear when this research will be done/implemented. Since your version already is an improvement over what is already in PL, I am very much in favour of making it more general now, and continuing to improve it iteratively down the line.
In other words: since this is already a net benefit over what PL users have access to, I don't think there is much disadvantage to making this generally available in the short term?
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.
@soranjh @josh146 I would suggest to go for 2 in this PR - this is the least work for you now, and it incurs only 100% non-user-facing changes in the future!
Then, echoing Josh, I would suggest to soon make a PR to call the private function (hacky for now) in Hamiltonian.simplify
if possible, or to otherwise improve that method with the insights gained here. We could combine it with making this Hamiltonian method return a new operator, but this breaks a few user-facing things so it is not trivial.
Lastly, we win more time for the ADR of how to do things properly.
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.
@mariaschuld makes the following proposal, which I think makes a lot of sense:
-
Continuing working on this PR
-
Once this PR is merged, improve the existing
Hamiltonian.simplify()
method with this code -
Finally, we can use what we learnt to (a) inform the ADR, and (b) help us decide any final UI/design decisions.
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 code looks great, no real suggestions.
But I wonder if following Maria's comment we should turn this into a more general qml.simplify()
function, which I think is a great idea
|
||
return qml.Hamiltonian(coeffs, ops) | ||
|
||
|
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 PR is literally adding a function that simplifies any input Hamiltonian. As far as I can tell, it should be possible to reproduce in the Hamiltonian class, or as you suggest, having qml.simplify(H)
. I really like this latter suggestion actually
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.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.
Just leaving a couple of suggestions and clarifications. Everything else looks good to me!
Context:
Add a modified version of the simplify function to the
hf
module. This functions combines redundant terms in a Hamiltonian and eliminates terms with zero coefficients.Description of the Change:
Function
_simplify
is added toqml.hf.hamiltonian
.Benefits:
The new function makes construction of molecular Hamiltonians more efficient. For LiH, as an example, the time to construct the Hamiltonian is reduced from ~200 s to ~10 s.
Possible Drawbacks:
Related GitHub Issues: