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

Add the support class for the Adjoint Jacobian to the new device #907

Merged
merged 49 commits into from
Sep 17, 2024

Conversation

LuisAlfredoNu
Copy link
Contributor

@LuisAlfredoNu LuisAlfredoNu commented Sep 11, 2024

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
    tests 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 .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:
Migrate LightningGPU to the new device API

Description of the Change:
Create the adjoint-jacobian class for the new device API to achieve the jacobian and vjp methods

Benefits:
Unlocking the adjoint-jacobian capabilities with the new device API for LGPU

Possible Drawbacks:

Related GitHub Issues:

Freezzed PR ⚠️ ❄️

To make a smooth integration of LightningGPU with the new device API, we set the branch gpuNewAPI_backend as the base branch target for future developments related to this big task.

The branch gpuNewAPI_backend has the mock of all classes and methods necessary for the new API. Also, several tests were disabled with

if device_name == "lightning.gpu":
    pytest.skip("LGPU new API in WIP.  Skipping.",allow_module_level=True)

However, these tests will unblocked as the implementation progresses.

After all the developments for integrating LightningGPU with the new API have been completed then the PR will be open to merge to master

[sc-70936] [sc-70939]

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@LuisAlfredoNu LuisAlfredoNu added the urgent Mark a pull request as high priority label Sep 11, 2024
@LuisAlfredoNu LuisAlfredoNu marked this pull request as ready for review September 16, 2024 22:57
@LuisAlfredoNu LuisAlfredoNu requested a review from a team September 16, 2024 22:58
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (gpuNewAPI_backend@fdc3f32). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ylane_lightning/lightning_gpu/_adjoint_jacobian.py 96.96% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##             gpuNewAPI_backend     #907   +/-   ##
====================================================
  Coverage                     ?   76.06%           
====================================================
  Files                        ?       28           
  Lines                        ?     2202           
  Branches                     ?        0           
====================================================
  Hits                         ?     1675           
  Misses                       ?      527           
  Partials                     ?        0           

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

@AmintorDusko
Copy link
Contributor

Hey @LuisAlfredoNu, is this PR ready for review? Apparently, we have several test fails.

@LuisAlfredoNu
Copy link
Contributor Author

Hey @LuisAlfredoNu, is this PR ready for review? Apparently, we have several test fails.

@AmintorDusko
Sorry, I realize that right now it has several errors after the merge of the last commit of simulate branch 😞. I will fix it and call to review it again. 😄

@AmintorDusko
Copy link
Contributor

Hey @LuisAlfredoNu, is this PR ready for review? Apparently, we have several test fails.

@AmintorDusko Sorry, I realize that right now it has several errors after the merge of the last commit of simulate branch 😞. I will fix it and call to review it again. 😄

Thank you, @LuisAlfredoNu!

@@ -302,7 +302,6 @@ def __init__( # pylint: disable=too-many-arguments
shots=None,
batch_obs=False,
# Kokkos arguments
sync=True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The var sync is no longer needed in LK.

): # pylint: disable=too-many-arguments

super().__init__(num_wires, dtype)

self._device_name = "lightning.kokkos"

self._kokkos_config = {}
self._sync = sync
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In LK, any method requires the var sync

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.

Nice work! Thanks @LuisAlfredoNu! Happy to approve 💯

Comment on lines -19 to +46
import pennylane as qml
from pennylane.tape import QuantumTape
Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

Great job!

@LuisAlfredoNu LuisAlfredoNu merged commit a7c4b09 into gpuNewAPI_backend Sep 17, 2024
75 checks passed
@LuisAlfredoNu LuisAlfredoNu deleted the gpuNewAPI_AdjJaco branch September 17, 2024 19:08
@LuisAlfredoNu LuisAlfredoNu restored the gpuNewAPI_AdjJaco branch September 17, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
urgent Mark a pull request as high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants