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

Remove Tensor and Hamiltonian in python frontend for lightning #994

Merged
merged 15 commits into from
Nov 13, 2024

Conversation

lillian542
Copy link
Contributor

@lillian542 lillian542 commented Nov 12, 2024

We are removing the Tensor and Hamiltonian classes, so we should remove their (minimal) remaining references in the Lightning python code.

Blocks running CI for the PL PR #6548 to remove legacy_opmath, since every test that uses lightning.qubit fails with Tensor removed.

[sc-77523]

@lillian542 lillian542 changed the title Remove Tensor and Hamiltonian in lightning Remove Tensor and Hamiltonian in python frontend for lightning Nov 12, 2024
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 92.14%. Comparing base (4659093) to head (ed37123).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pennylane_lightning/core/lightning_base.py 0.00% 4 Missing ⚠️
pennylane_lightning/core/_measurements_base.py 25.00% 3 Missing ⚠️
pennylane_lightning/core/_serialize.py 75.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (4659093) and HEAD (ed37123). Click for more details.

HEAD has 30 uploads less than BASE
Flag BASE (4659093) HEAD (ed37123)
38 8
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
- Coverage   98.00%   92.14%   -5.86%     
==========================================
  Files         231      104     -127     
  Lines       36387    16266   -20121     
==========================================
- Hits        35662    14989   -20673     
- Misses        725     1277     +552     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lillian542 lillian542 marked this pull request as draft November 12, 2024 16:17
@lillian542 lillian542 marked this pull request as ready for review November 12, 2024 17:38
@maliasadi maliasadi requested a review from a team November 12, 2024 18:26
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks @lillian542 🥳

Copy link
Member

@multiphaseCFD multiphaseCFD left a comment

Choose a reason for hiding this comment

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

Thanks @lillian542 . Awesome. Just wondering if same action is required for lightning_tensor?

@lillian542
Copy link
Contributor Author

@multiphaseCFD thanks, good question! It doesn't import or any custom code for Hamiltonian or Tensor, so I had discounted it, but I went back and checked and "Hamiltonian" was still on the list of supported ops, which it preferably shouldn't be. I've updated that now; as far as I can see, nothing else in the code for that device is specific to the legacy opmath.

@multiphaseCFD
Copy link
Member

@multiphaseCFD thanks, good question! It doesn't import or any custom code for Hamiltonian or Tensor, so I had discounted it, but I went back and checked and "Hamiltonian" was still on the list of supported ops, which it preferably shouldn't be. I've updated that now; as far as I can see, nothing else in the code for that device is specific to the legacy opmath.

Thanks @lillian542 . Could you please also add the ci:use-gpu-runner label to this PR and then trigger CIs? Then changed lines in lightning.gpu and lightning.tensor can be tested.

@multiphaseCFD multiphaseCFD added ci:use-gpu-runner Enable usage of GPU runner for this Pull Request and removed ci:use-gpu-runner Enable usage of GPU runner for this Pull Request labels Nov 12, 2024
Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

@maliasadi and @lillian542, we have some C++ tests that emulate the data structure of a Tensor and/or Hamiltonian. Are we renaming, updating or removing these tests? Or are we going to leave it for now?
Lilian, please refresh my mind, what will be substituting these in the new opmath?

@lillian542
Copy link
Contributor Author

@multiphaseCFD if we need to make changes in the C++ backend, would it be alight for that to happen in a different PR? Since PennyLane runs tests with lightning.qubit, the CI for the PennyLane PR that removes legacy operator arithmetic is blocked by this.

The Tensor class will now be fully replaced by Prod. The default has been Prod for a few months, but we have continued to support Tensor if users opt in.

The Hamiltonian class will now be replaced by LinearCombination and Sum (which one depends on how the user creates the operator). LinearCombination is a subclass of Sum with some minimal, generally user-facing differences. Like with Tensor, this behaviour has been the default for a few months, and only users who 'opted out' of the new behaviour have still been using the legacy opmath support.

@multiphaseCFD
Copy link
Member

multiphaseCFD commented Nov 13, 2024

@multiphaseCFD if we need to make changes in the C++ backend, would it be alight for that to happen in a different PR? Since PennyLane runs tests with lightning.qubit, the CI for the PennyLane PR that removes legacy operator arithmetic is blocked by this.

The Tensor class will now be fully replaced by Prod. The default has been Prod for a few months, but we have continued to support Tensor if users opt in.

The Hamiltonian class will now be replaced by LinearCombination and Sum (which one depends on how the user creates the operator). LinearCombination is a subclass of Sum with some minimal, generally user-facing differences. Like with Tensor, this behaviour has been the default for a few months, and only users who 'opted out' of the new behaviour have still been using the legacy opmath support.

Thanks @lillian542 . Good question. I would suggest to create a new PR to work on the C++ layer changes as long as current changes in the frontend do not break the pipeline. But I would like to ask @maliasadi to decide.

@maliasadi
Copy link
Member

we have some C++ tests that emulate the data structure of a Tensor and/or Hamiltonian. Are we renaming, updating or removing these tests? Or are we going to leave it for now?

@AmintorDusko @multiphaseCFD @lillian542 We don't need any changes in C++ right now as Lightning and Catalyst continue serializing qml.pauli.PauliWord --> TensorProduct and qml.pauli.PauliSentence --> Hamiltonian.
We will simplify the observable serialization implementation and extend the implementation of C++ observables to adhere the new PL obs abstractions in both Lightning and Catalyst.

Copy link
Member

@multiphaseCFD multiphaseCFD left a comment

Choose a reason for hiding this comment

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

Thanks @lillian542 for the nice work! Happy to approve!

@lillian542 lillian542 merged commit 94577a8 into master Nov 13, 2024
71 of 73 checks passed
@lillian542 lillian542 deleted the remove_legacy_opmath branch November 13, 2024 15:54
multiphaseCFD pushed a commit that referenced this pull request Nov 13, 2024
We are removing the Tensor and Hamiltonian classes, so we should remove
their (minimal) remaining references in the Lightning python code.

Blocks running CI for the [PL PR
legacy_opmath, since every test that uses `lightning.qubit` fails with
Tensor removed.

---------

Co-authored-by: ringo-but-quantum <github-ringo-but-quantum@xanadu.ai>
multiphaseCFD pushed a commit that referenced this pull request Nov 13, 2024
We are removing the Tensor and Hamiltonian classes, so we should remove
their (minimal) remaining references in the Lightning python code.

Blocks running CI for the [PL PR
legacy_opmath, since every test that uses `lightning.qubit` fails with
Tensor removed.

---------

Co-authored-by: ringo-but-quantum <github-ringo-but-quantum@xanadu.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:use-gpu-runner Enable usage of GPU runner for this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants