-
Notifications
You must be signed in to change notification settings - Fork 585
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
print of vqe.hamiltonian sorts terms by number of wires and coefficients #981
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #981 +/- ##
==========================================
- Coverage 99.09% 98.11% -0.99%
==========================================
Files 142 145 +3
Lines 10439 10943 +504
==========================================
+ Hits 10345 10737 +392
- Misses 94 206 +112
Continue to review full report at Codecov.
|
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.
A nice improvement, thanks @albi3ro!
I might comment on each independently.
-
Sorting by the number of wires in each term is a great idea. This is something that has bothered me for a while, and I'm glad this is fixed.
-
I'm much more hesitant on including tabs. I find tabs are not consistent between editors, or even operating systems, and what looks nice on one users system might look worse on another- case in point, the PR comment example. I suggest reverting the tab change to be a single space, or, if possible, seeing if Python string formatting provides options for alignment.
Finally, it looks like the logic changes might need some new tests.
Will wait on Issue #982 to write up the tests. |
0d710f6
to
d4d8cd5
Compare
Just updated this to just get it out of the way and clear the backlog. |
Hi @albi3ro, just double checking the status of this PR - is it ready for re-review, is it still WIP, or has it been superseded? |
@josh146 I can't even find what change you requested anymore, but this should finally be good to merge 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.
Thanks @albi3ro! Looks good to merge now - I think my only requested change was removing the tab 😆
"(1.5) [Z0]\n+ (2.0) [Y2]", | ||
"(-0.1) [Hermitian0'1]\n+ (0.5) [Y0]", | ||
"(0.5) [X0]\n+ (1.2) [X0 X1]", | ||
" (1.0) [Hermitian0'1]", |
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.
Could changing the spacing have any side effects? Maybe demos outputs/existing docstrings might not match any more?
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.
Demo's won't match up anymore, but also because the terms will be in different orders.
docstrings might be an issue. I've been thinking as well about going through docstrings and making sure code samples still work. I think there may be multiple places code examples need to get updated. I made an Issue the other day about circuit drawing in the permute template.
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 saw, that was a good catch!
Co-authored-by: Josh Izaac <josh146@gmail.com>
I like this change 👍 One place where this may be a surprise is when a Hamiltonian is used with ApproxTimeEvolution, since this operation uses the ordered terms in the Hamiltonian which may not match the printed terms. For example, consider >>> import pennylane as qml
>>> h = qml.Hamiltonian([1, -1, -2], [qml.PauliX(0), qml.PauliZ(0), qml.PauliY(0)])
>>> print(h.terms)
([1, -1, -2], [PauliX(wires=[0]), PauliZ(wires=[0]), PauliY(wires=[0])])
>>> print(h)
(-2) [Y0]
+ (-1) [Z0]
+ (1) [X0] So |
This is a minor aesthetic change to improve readability on printing objects of type
pennylane.vqe.vqe.Hamiltonian
.Currently, the H2 hamiltonian from the VQE tutorial prints as:
With this change, the terms will be sorted in the number of wires, and a tab
\t
attempts to line up the operation terms.This improves the user's ability to compare like terms. For example, now I can much more easily see that
[Z0 Z2]
and[Z1 Z3]
have the same coefficient.This new function doesn't distinguish trivial wires acted upon by the identity. For example, after sorting by the coefficient, the identity term gets mixed into the single wire terms. Fixing that would add more complexity to a fairly simple function, so I don't know if it's worth figuring that out.