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.

Sign up for GitHub

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

Jump to bottom

Update to support lightning (new device API) & the PL operator arithmetic update #638

Merged
merged 43 commits into from
Apr 22, 2024
Merged

Update to support lightning (new device API) & the PL operator arithmetic update #638

merged 43 commits into from
Apr 22, 2024

Conversation

rmoyard
Copy link
Contributor

@rmoyard rmoyard commented Apr 4, 2024 •
edited
Loading

Context:

  • PL updated the operator arithmetic
  • PL-Lightning switched to the new device API

Description of the Change:

  • Update the transform pipeline for the new device API
  • Create a decompose compatible with Catalyst
  • Update for the operator arithmetic
  • Pin PL to master and PL-lightning to dev-30

Benefits:

  • Catalyst supports the updated version of PL and Ligthning

@rmoyard
Copy link
Contributor Author

rmoyard commented Apr 12, 2024

Changes needed: PennyLaneAI/pennylane#5506 PennyLaneAI/pennylane#5478

@rmoyard rmoyard changed the title Update to support lightning for the new device API Update to support lightning for the new device API & the PL operator arithmetic update Apr 12, 2024
@rmoyard rmoyard requested a review from lillian542 April 18, 2024 18:34
@rmoyard
Copy link
Contributor Author

rmoyard commented Apr 18, 2024

[sc-60562]

@rmoyard
Copy link
Contributor Author

rmoyard commented Apr 18, 2024

[sc-60606]

@rmoyard
Copy link
Contributor Author

rmoyard commented Apr 18, 2024

[sc-60561]

maliasadi
maliasadi reviewed Apr 18, 2024
View reviewed changes
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.

Thanks @rmoyard! 🥳

.dep-versions Outdated Show resolved Hide resolved
doc/requirements.txt Show resolved Hide resolved
frontend/catalyst/jax_tracer.py Show resolved Hide resolved
frontend/test/lit/test_custom_ops.py Outdated Show resolved Hide resolved
rmoyard and others added 2 commits April 18, 2024 16:01
@rmoyard
Frozen set to list/set instead of kokkos
b4feb60
@rmoyard @maliasadi
Apply suggestions from code review
1312c83 
Co-authored-by: Ali Asadi <10773383+maliasadi@users.noreply.github.com>
@rmoyard rmoyard requested a review from maliasadi April 18, 2024 20:03
maliasadi
maliasadi approved these changes Apr 18, 2024
View reviewed changes
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.

All good from my side 💯 you may want to wait for the second approval from the compilation team.

@erick-xanadu
Copy link
Contributor

@rmoyard the shortcut ticket here says that this issue is related to decomposing nested operations like control and adjoint, but there are no tests that check for the decomposition of them. Is there an error in the shortcut ticket assigned to this?

@rmoyard
Copy link
Contributor Author

rmoyard commented Apr 19, 2024

@erick-xanadu They are multiple shortcut tickets (this is the part one for the decompose ticket), the unit tests for decompose will be added by @lillian542 as the part 2, she is currently working on making the solution better.

erick-xanadu
erick-xanadu requested changes Apr 19, 2024
View reviewed changes
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_tracer.py Show resolved Hide resolved
frontend/catalyst/preprocess.py Show resolved Hide resolved
frontend/catalyst/preprocess.py Show resolved Hide resolved
frontend/catalyst/preprocess.py Show resolved Hide resolved
frontend/test/lit/test_quantum_control.py Outdated Show resolved Hide resolved
erick-xanadu
erick-xanadu approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

Thanks Romain!

@rmoyard rmoyard merged commit 2f8a966 into main Apr 22, 2024
37 of 38 checks passed
@rmoyard rmoyard deleted the lightning_new_api branch April 22, 2024 13:38
@lillian542 lillian542 mentioned this pull request May 7, 2024
Merged
lillian542 added a commit that referenced this pull request May 15, 2024
@lillian542
Decompose nested tapes in HybridOps Pt 2 (#728)
5d7d1fd 
**Context:**
An initial path-of-least-resistance implementation of `decompose` was
added in #638, because it was necessary to complete the task at hand in
that PR. However, as it wasn't the main focus of that PR, and it was
merged with the plan to fix several things if possible. This PR
implements the "cleaner", more updated version of the transform. In the
initial implementation:

1. There was no testing for the added decomposition of HybridOp tapes
(high priority)
2. A `visited` property was temporarily added to the HybridOp class to
make things easier to implement, and should be removed (if possible)
3. A lot of the code involved was almost, but not quite, identical to
existing code in PL, and the duplication should be reduced (if possible)

**Description of the Change:**
Tests are added, and the code adjusted to fix any issues that came up.
The code was refactored to use most of the comparable functionality in
PennyLane instead of duplicating it in catalyst, and to remove the need
for the `visited` property on `HybridOps`.

**Benefits:**
We made the changes to make this work more cleanly as discussed during
the review process for #638. We avoid duplicate code. We have tests that
confirm that the decomposition of nested tapes is working as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maliasadi maliasadi maliasadi approved these changes

@erick-xanadu erick-xanadu erick-xanadu approved these changes

@dime10 dime10 Awaiting requested review from dime10

@lillian542 lillian542 Awaiting requested review from lillian542

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rmoyard @erick-xanadu @maliasadi @dime10 @lillian542