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

Simplify VJP/JVP/Grad/Jacobian frontend #508

Merged
merged 23 commits into from
Feb 14, 2024
Merged

Simplify VJP/JVP/Grad/Jacobian frontend #508

merged 23 commits into from
Feb 14, 2024

Conversation

rmoyard
Copy link
Contributor

@rmoyard rmoyard commented Feb 13, 2024

Description of the Change:
Simplify the unflatten of VJP/JVP/grad/Jacobian and covers more edge cases.

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a308574) 99.55% compared to head (9103f2c) 99.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #508      +/-   ##
==========================================
- Coverage   99.55%   99.55%   -0.01%     
==========================================
  Files          43       43              
  Lines        7824     7821       -3     
  Branches      541      537       -4     
==========================================
- Hits         7789     7786       -3     
  Misses         18       18              
  Partials       17       17              

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

@rmoyard rmoyard changed the title Clean returns VJP/JVP Simplify VJP/JVP frontend Feb 13, 2024
@rmoyard rmoyard changed the title Simplify VJP/JVP frontend Simplify VJP/JVP/Grad/Jacobian frontend Feb 13, 2024
@rmoyard rmoyard requested a review from dime10 February 13, 2024 22:09
@rmoyard rmoyard marked this pull request as ready for review February 13, 2024 22:09
@josh146
Copy link
Member

josh146 commented Feb 14, 2024

This will be really nice to have in 😎

@rmoyard I think a lot of docstrings/codeblocks (especially the quickstart, sharp bits page, etc.) will likely also need to be updated 🤔

@rmoyard
Copy link
Contributor Author

rmoyard commented Feb 14, 2024

@josh146 I am double checking the docs

@rmoyard
Copy link
Contributor Author

rmoyard commented Feb 14, 2024

I have found some changes in the quickstart and sharp bits, and updated them

@rmoyard rmoyard added the frontend Pull requests that update the frontend label Feb 14, 2024
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for going over those PRs again 👍 I think we should update the docstrings of the functions as well, what do you think?

Additionally, were the original PRs introduced as a breaking change in the changelog? (This PR also needs to be added to those changelog entries.)

frontend/catalyst/pennylane_extensions.py Outdated Show resolved Hide resolved
frontend/catalyst/pennylane_extensions.py Outdated Show resolved Hide resolved
frontend/catalyst/pennylane_extensions.py Outdated Show resolved Hide resolved
frontend/catalyst/pennylane_extensions.py Show resolved Hide resolved
@rmoyard
Copy link
Contributor Author

rmoyard commented Feb 14, 2024

@dime10 I have added a note in each function that we support function that returns any pytree-like shape

@rmoyard rmoyard requested a review from dime10 February 14, 2024 16:50
@rmoyard rmoyard requested a review from dime10 February 14, 2024 18:21
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Looks good on my side 👍

@rmoyard rmoyard merged commit 839a0fd into main Feb 14, 2024
36 checks passed
@rmoyard rmoyard deleted the clean_vjp_jvp branch February 14, 2024 19:05
Comment on lines 638 to +640
>>> workflow(jnp.array([2.0, 1.0]))
array([[-1.32116540e-07, 1.33781874e-07],
[-4.20735506e-01, 4.20735506e-01]])
array([[ 1.74393425e-16 4.54648713e-01]
[-1.74393425e-16 -4.54648713e-01]])
Copy link
Member

Choose a reason for hiding this comment

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

do we expect the gradient to change 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have copied the wrong one normally there is just a transposition, sorry for that. And this was introduced in a previous PR. Open a Pr shortly.

[[ 3.48786850e-16 -4.20735492e-01]
 [-8.71967125e-17  4.20735492e-01]]

@dime10 dime10 mentioned this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Pull requests that update the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants