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
feat: Updated repr for ParametrizedHamiltonian #4176
feat: Updated repr for ParametrizedHamiltonian #4176
Conversation
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.
Hello @anushkrishnav thank you for your contribution!
This already looks like it is on a good way :-)
There are two issues with the current code:
- the index i is not displaying the correct number
- callable class objects raise an error because they dont have a
__name__
attribute
Additionally, it seems there is a failing test in tests/pulse/test_parametrized_hamiltonian.py::TestInitialization::test__repr__
You can update this test, and I would suggest adding another test to check the new feature behaves as you intend it to. In order to check the test is passing, you can run python -m pytest tests/pulse/test_parametrized_hamiltonian.py
(or any other relevant file) in the terminal before pushing.
The following is optional:
There is a special subclass of ParametrizedHamiltonian
, HardwareHamiltonian
. Let me tell you what the situation is and how it is handled in HardwareHamiltonian
.
For transmon or rydberg systems, the driving term of the Hamiltonian typically has the form
When implementing this in pennylane right now, this yields two separate terms _reorder_parameters
function that reorders the parameters that are passed (see https://github.com/PennyLaneAI/pennylane/blob/master/pennylane/pulse/hardware_hamiltonian.py#L237 for rydberg systems and https://github.com/PennyLaneAI/pennylane/blob/master/pennylane/pulse/transmon.py#L502 for transmons.
That means, the string representation of HardwareHamiltonian
(which inhertis from ParametrizedHamiltonian
) would have to be overwritten with a slightly modified function that takes that reordering into account.
It is slightly more tricky, so optional. Let me know if you are interested and I can provide you with more info and help.
Hey thanks for the detailed review @Qottmann I will start working on the requested changes, Will update the tests as well. |
I have made the requested updates @Qottmann |
Yeh, the optional one sounds interesting do share more info I will work on it as an individual PR, I am currently looking at the files you share to get a better grasp of what's actually happening and the reordering , |
Codecov Report
@@ Coverage Diff @@
## master #4176 +/- ##
=======================================
Coverage 99.77% 99.77%
=======================================
Files 342 342
Lines 30709 30721 +12
=======================================
+ Hits 30639 30651 +12
Misses 70 70
|
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.
Getting there, great work @anushkrishnav 💪
Note that lines 253 and 255 are currently not covered by the tests (this is what codecov checks and complains about in the test suite below). In order to fix that, you need to add tests that cover these cases. I.e. test the string representation with a ParametrizedHamiltonian
that contains a callable class object with a __name__
, and another test with a callable class object with no __name__
and assert that it outputs what you expect.
Some optional refinement (not needed for unitary hack, but I think it would make the PR overall nicer): Currently everything is returned in one line. It would be great to output terms in separate lines like is done in qml.Hamiltonian
, see ops/qubit/hamiltonian.py#LL490 for inspiration. See example here:
Since we are changing the string representation, we also need to make sure the outputs that were hardcoded in the docs are updated. This concerns, as far as I can see, the doc strings in:
- the module page https://docs.pennylane.ai/en/latest/code/qml_pulse.html
ParmetrizedHamiltonian
https://docs.pennylane.ai/en/latest/code/api/pennylane.pulse.ParametrizedHamiltonian.html
(Cant guarantee I didnt miss another public doc string in the pulse module)
I.e. this would need to be updated in the current docs:
(You can render docs locally by runningmake docs
to check them, or click on "details" in the readthedocs item in the test suite:)
If you want to do the case of HardwareHamiltonian
in a separate PR, already this PR should overwrite the __repr__
to do the default because it currently outputs the wrong string. I will write a separate message to provide more details to continue there :)
Re where To allow users to be flexible in whether they want The string representation would basically have to follow the same logic as in https://github.com/PennyLaneAI/pennylane/blob/master/pennylane/pulse/transmon.py#L502 i.e. check whether it is an instance of this special callable class, and if yes count the number of callables and accordingly increase the index that is being displayed. So ideally the string representation for the following Hamiltonian would look like this
The part about the annihilation and creation operators being explicitly constructed as |
I don't think we will require the else condition since it deals with only 3 situations from what I understand, The coeff are numerical values or a function with name or an object with no name but can be obtained using class.name
(2.0*(PauliX(wires=[0]))) + (f1(params_0, t)(PauliY(wires=[0]))) + (f2(params_1, t)(PauliZ(wires=[0]))) + ((params_2, t)*(PauliX(wires=[0]))) |
As for the docs I just need to update the docstring right? I did a search and updated the docstring I can find |
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 the update @anushkrishnav 🚀
The line breaks make it very clean, nice!
There are missing outputs in the docstring of ParametrizedHamiltonian
We cant merge the PR as long as the transmon and rydberg Hamiltonians are producing erroneous outputs. Since HardwareHamiltonian inherits from ParametrizedHamiltonian
, it is using the same __repr__
method. But in that case, we want the default back because the logic that you added does not apply to those Hamiltonians as described and discussed in the potential follow-up PR.
Note also that currently pylint and black are failing! |
Yeh I saw the pylint my next commit will fix that |
8f581af
to
b60d4ec
Compare
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 the update @anushkrishnav 💪
Looks almost good to go!
There are some missing output changes in the docstrings!
In the rydberg_interaction
docstring:
in rydberg_drive
in transmon_interaction
We require two approving reviews before merging, another colleague will do a code-review in the following days. There may be things that I have missed and further changes necessary for them, but overall it is looking good to me 👍
I noticed it much later, pushing in the changes sorry about the docsting |
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 is looking great, thanks! 🎉
I only have a couple of very minor comments:
There is an example in pennylane/ops/functions/dot.py
that still uses the old representation in the docstring. I also see a few docstring examples in the transmon.py
and rydberg.py
files that show ParametrizedHamiltonian
instead of HardwareHamiltonian
.
Don't forget to add a change-log entry and include yourself in the list of contributors!
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.
Nice! Thanks @anushkrishnav 🎉
Thank you ! I will look into Hardware soon , I think I will be able to get it done too |
Before submitting
Please complete the following checklist when submitting a PR:
All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to the
test directory!
All new functions and code must be clearly commented and documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running
make docs
.Ensure that the test suite passes, by running
make test
.Add a new entry to the
doc/releases/changelog-dev.md
file, summarizing thechange, and including a link back to the PR.
The PennyLane source code conforms to
PEP8 standards.
We check all of our code against Pylint.
To lint modified files, simply
pip install pylint
, and thenrun
pylint pennylane/path/to/file.py
.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context:
The context of this change is to provide an informative string representation for the ParametrizedHamiltonian class in PennyLane.
Description of the Change:
The change involves updating the repr method of the ParametrizedHamiltonian class to generate a string representation that closely follows the example provided. This includes incorporating the parametrized functions and their corresponding indices in the expression, using the appropriate brackets.
Benefits:
The updated string representation will provide users with a more informative view of the ParametrizedHamiltonian object. It will clearly show the terms of the Hamiltonian, including any parametrized functions, their corresponding parameter indices, and the time variable.
Possible Drawbacks:
One potential drawback is that the string representation may become more complex for Hamiltonians with a large number of terms or deeply nested expressions. This could make it harder to read and understand for very complex Hamiltonians.
Related GitHub Issues:
#4088