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

Register Operator classes and pytrees #4458

Merged
merged 42 commits into from
Aug 16, 2023
Merged

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Aug 10, 2023

[sc-40555]

This PR adds a pennylane/pytrees.py module that contains the code for registering a custom object with a pytree framework. The only framework is currently jax, but we can extend that in the future.

The dynamic typing in Adjoint.__new__ caused some issues, so that was slightly rewritten, and is no longer causing issues.

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #4458 (98f1260) into master (af1eba4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4458   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files         377      378    +1     
  Lines       34180    34197   +17     
=======================================
+ Hits        34084    34101   +17     
  Misses         96       96           
Files Changed Coverage Δ
pennylane/operation.py 97.41% <100.00%> (+0.01%) ⬆️
pennylane/ops/op_math/adjoint.py 99.19% <100.00%> (-0.03%) ⬇️
pennylane/pytrees.py 100.00% <100.00%> (ø)

Copy link
Contributor

@vincentmr vincentmr left a comment

Choose a reason for hiding this comment

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

Nicely done. Thanks @albi3ro .

Copy link
Contributor

@BorjaRequena BorjaRequena left a comment

Choose a reason for hiding this comment

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

Looking good @albi3ro! Thanks for taking care of this pytreefication, it will allow us to do a lot of cool things with operators 🚀 Which makes me wonder if we should find a compact way to test those, like testing jax.jit, jax.grad and jax.vmap on some operator functions, if that even makes sense 🤔

Also, could you add this line that we could not include in the previous PR? 😄

tests/ops/qubit/test_parametric_ops.py Show resolved Hide resolved
pennylane/operation.py Show resolved Hide resolved
pennylane/pytrees.py Show resolved Hide resolved
pennylane/pytrees.py Show resolved Hide resolved
tests/test_operation.py Show resolved Hide resolved
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

very exciting, looks good! do we want to add any details about ops-as-pytrees to the docs, perhaps just in the internal section? I'm unsure myself.

Also unrelated but I realized while checking out the Adjoint docs - the contents of the "Developer Details" section of the docs are not indented, so they don't hide in the HTML drop-down when rendered. Could you indent them here?

Co-authored-by: BorjaRequena <59647767+BorjaRequena@users.noreply.github.com>
@josh146
Copy link
Member

josh146 commented Aug 16, 2023

@timmysilv -- could be a good section to add to the https://docs.pennylane.ai/en/stable/development/adding_operators.html page?

@albi3ro
Copy link
Contributor Author

albi3ro commented Aug 16, 2023

very exciting, looks good! do we want to add any details about ops-as-pytrees to the docs, perhaps just in the internal section? I'm unsure myself.

Also unrelated but I realized while checking out the Adjoint docs - the contents of the "Developer Details" section of the docs are not indented, so they don't hide in the HTML drop-down when rendered. Could you indent them here?

@josh146 @timmysilv

https://docs.pennylane.ai/en/latest/code/api/pennylane.operation.Operator.html#serialization

@albi3ro albi3ro enabled auto-merge (squash) August 16, 2023 14:07
@josh146
Copy link
Member

josh146 commented Aug 16, 2023

Looks good @albi3ro! Maybe out of scope for this PR, but it looks like in the sentence

See the Operator._flatten and Operator._unflatten methods for more information.

the links don't work 🤔 Is it because they are private methods/are not showing in the docs?

@albi3ro
Copy link
Contributor Author

albi3ro commented Aug 16, 2023

Looks good @albi3ro! Maybe out of scope for this PR, but it looks like in the sentence

See the Operator._flatten and Operator._unflatten methods for more information.

the links don't work 🤔 Is it because they are private methods/are not showing in the docs?

Private methods.

@albi3ro albi3ro added the merge-ready ✔️ All tests pass and the PR is ready to be merged. label Aug 16, 2023
@josh146
Copy link
Member

josh146 commented Aug 16, 2023

Got it, thanks @albi3ro. In that case, we may need to revisit that docstring, to make it easier to direct people looking for more information (or, if we do want Operator._flatten etc. to be documented, make them public methods)

@albi3ro
Copy link
Contributor Author

albi3ro commented Aug 16, 2023

Got it, thanks @albi3ro. In that case, we may need to revisit that docstring, to make it easier to direct people looking for more information (or, if we do want Operator._flatten etc. to be documented, make them public methods)

I wonder if we could consider making private methods on Operator show up in the documentation. They are private because nothing outside of the class should touch them, but they are relevant to anyone trying to implement the class.

@albi3ro albi3ro disabled auto-merge August 16, 2023 15:42
@albi3ro albi3ro enabled auto-merge (squash) August 16, 2023 15:57
@albi3ro albi3ro merged commit 9aab699 into master Aug 16, 2023
39 checks passed
@albi3ro albi3ro deleted the register-operator-pytree branch August 16, 2023 17:02

has_jax = True
try:
import jax.tree_util as jax_tree_util
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can avoid importing JAX with PennyLane?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

And i saw no import time difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a shame, it locks us in to having to import JAX at the same time as PennyLane.

mlxd pushed a commit that referenced this pull request Aug 23, 2023
* more custom definitions

* more templates

* more tests

* more tests for templates

* Update doc/development/deprecations.rst

* more tests

* final round of template tests

* remove qml.equal change

* Update pennylane/templates/embeddings/angle.py

* fix op_math tests

* Update pennylane/templates/layers/random.py

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

* Update doc/development/deprecations.rst

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

* finishing touches

* some more tests and fixes

* fix remaining tests, lint

* fix test lint

* register operator with jax

* register with torch too

* tidying

* register operations as pytrees

* merge

* adding tests

* Removing changes we didn't need to make to test_adjoint

* sphinx, black, and pylint

* adjoint changes and pylint [skip ci]

* test for adjoint pickling

* black and pylint

* Update tests/ops/qubit/test_parametric_ops.py

Co-authored-by: BorjaRequena <59647767+BorjaRequena@users.noreply.github.com>

* indent developer details section

* black

* add multiprocessing with adjoint test

---------

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: BorjaRequena <59647767+BorjaRequena@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-ready ✔️ All tests pass and the PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants