Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a modified simplify function for building Hamiltonians #2103
Changes from all commits
8e0ce7b
98f25b6
b56a79c
4b9f715
9d19f9d
5608a0c
0e249a0
059788d
1a7ce23
a42cac5
da7182b
ebfbffb
ea4289e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 ofH
, which is dangerous. We'd like it to return a new operator. Another option isqml.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 actuallyThere 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 usingcompare
, one can simply useif 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
insidehf.hamiltonian
for now.So, I want your thoughts on these options:
hf.hamiltonian._simplify
toqml.simplify(H)
.qml.simplify(H)
accordingly.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 codeFinally, we can use what we learnt to (a) inform the ADR, and (b) help us decide any final UI/design decisions.