Skip to content
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

Move code from qml.utils.sparse_hamiltonian to Hamiltonian.sparse_matrix #3236 #3585

Merged
merged 24 commits into from
Feb 13, 2023
Merged

Move code from qml.utils.sparse_hamiltonian to Hamiltonian.sparse_matrix #3236 #3585

merged 24 commits into from
Feb 13, 2023

Conversation

12mB7693
Copy link
Contributor

@12mB7693 12mB7693 commented Dec 27, 2022

…trix, added a deprecation warning to qml.utils.sparse_hamiltonian

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 the
    change, 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 then
    run 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:

Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:

…trix, added a deprecation warning to qml.utils.sparse_hamiltonian
@codecov
Copy link

codecov bot commented Dec 27, 2022

Codecov Report

Merging #3585 (be9ce6c) into master (1774fe3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3585   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files         330      330           
  Lines       28943    28949    +6     
=======================================
+ Hits        28892    28898    +6     
  Misses         51       51           
Impacted Files Coverage Δ
pennylane/optimize/lie_algebra.py 100.00% <ø> (ø)
pennylane/devices/default_qubit.py 99.69% <100.00%> (-0.01%) ⬇️
pennylane/ops/functions/matrix.py 100.00% <100.00%> (ø)
pennylane/ops/qubit/hamiltonian.py 100.00% <100.00%> (ø)
pennylane/utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pennylane/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR @12mB7693 !

The tests for the original sparse_hamiltonian method in test_utils.py can be copied and repurposed to be added to tests/ops/qubit/test_hamiltonian.py.

We also need a changelog entry under "Deprecations". Don't forget to add your name to the list of contributors as well! The changelog can be found at: https://github.com/PennyLaneAI/pennylane/blob/master/doc/releases/changelog-dev.md

We will also need to add add entry to the doc/developement/deprecations.rst file. Since are now developing for v0.29, let's schedule qml.utils.sparse_hamiltonian for removal in v0.31.

I've left comments with suggestions to fix the various linting and formatting comments. If you want to learn more about running the various tools we use to standardize code formatting, you can read our pull request guide.

We can also do this is a later PR if necessary, but we can now update the three places in PennyLane that use qml.utils.sparse_hamiltonian:

greater and others added 4 commits January 14, 2023 19:41
…cated function to the deprecations list, added tests
Added test that checks if deprecation warning has been raised, updated changelog
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. This looks in really good shape.

I'd just like to make a comment about handling the wire order at the end with expand_matrix, versus constructing the sparse matrix with the wire order from the start.

Even for a simple case like:

H = 2.0 * qml.PauliX(0) @ qml.PauliY(1) @ qml.PauliZ(2) @ qml.PauliX(3)
%timeit qml.utils.sparse_hamiltonian(H, wires = list(reversed(H.wires)))

You'll see that using that handling the wire order at the end is quite a bit slower. We want to construct to the specific wire order from the very beginning.

So instead of using self.wires inHamiltonian.sparse_matrix, we want to use wires = wire_order or self.wires.

Does that make sense?

@12mB7693
Copy link
Contributor Author

@albi3ro Thank you for reviewing what I did and for your comments, that helped me a lot.
I think I get what you mean and I will push an update soon.
Shall I actually also update the tests, where the old function qml.utils.sparse_hamiltonian(..) is called?

@albi3ro
Copy link
Contributor

albi3ro commented Jan 18, 2023

@albi3ro Thank you for reviewing what I did and for your comments, that helped me a lot.
I think I get what you mean and I will push an update soon.
Shall I actually also update the tests, where the old function qml.utils.sparse_hamiltonian(..) is called?

Updating the tests where qml.utils.spase_hamiltonian is called (outside of unit tests) would be great! But I won't necessarily block approval based on that being accomplished.

@12mB7693
Copy link
Contributor Author

@albi3ro Thank you for reviewing what I did and for your comments, that helped me a lot.
I think I get what you mean and I will push an update soon.
Shall I actually also update the tests, where the old function qml.utils.sparse_hamiltonian(..) is called?

Updating the tests where qml.utils.spase_hamiltonian is called (outside of unit tests) would be great! But I won't necessarily block approval based on that being accomplished.

I'm not sure if I understood you correctly regarding the updates of the tests. Do you mean with "outside of unit tests" that I should not update the unit tests?

@12mB7693
Copy link
Contributor Author

Not sure why the documentation check fails, I ran "make doc" locally and I get the warning
releases/changelog-dev.md:391: WARNING: Definition list ends without a blank line; unexpected unindent.
However, I do not see how to fix it.

@albi3ro albi3ro changed the title [WIP] Move code from qml.utils.sparse_hamiltonian to Hamiltonian.sparse_matrix #3236 Move code from qml.utils.sparse_hamiltonian to Hamiltonian.sparse_matrix #3236 Jan 26, 2023
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @12mB7693 ! 🎉

Looks good to me. Glad to finally have this tidied up :)

Sphinx may still be causing some problems, but I'll try and fix then if I can.

Excellent work :)

@12mB7693
Copy link
Contributor Author

No problem, I'm glad that I could help. This task was definitely a good starting point as a new contributor.
Thank you for your support @albi3ro!

@albi3ro albi3ro enabled auto-merge (squash) February 13, 2023 18:25
@albi3ro albi3ro added this to the Release v0.29 milestone Feb 13, 2023
@albi3ro albi3ro enabled auto-merge (squash) February 13, 2023 20:42
@albi3ro albi3ro merged commit 162472f into PennyLaneAI:master Feb 13, 2023
mudit2812 pushed a commit that referenced this pull request Apr 13, 2023
…rix #3236 (#3585)

* Moved code from qml.utils.sparse_hamiltonian to Hamiltonian.sparse_matrix, added a deprecation warning to qml.utils.sparse_hamiltonian

* Replaced function calls of deprecated sparse_hamiltonian, added deprecated function to the deprecations list, added tests

* Added test and updated changelog

Added test that checks if deprecation warning has been raised, updated changelog

* changed handling of wire_order

* simplified code

* Removed unnecessary import, fixed documentation

* Fixed error

* Update doc/releases/changelog-dev.md

* Update doc/releases/changelog-dev.md

* Apply suggestions from code review

* Update doc/releases/changelog-dev.md

* Update doc/releases/changelog-dev.md

---------

Co-authored-by: 12mB7693 <abc@def.com>
Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: Christina Lee <chrissie.c.l@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants