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

Make templates valid Pytrees #5698

Merged
merged 30 commits into from
May 30, 2024
Merged

Make templates valid Pytrees #5698

merged 30 commits into from
May 30, 2024

Conversation

dwierichs
Copy link
Contributor

@dwierichs dwierichs commented May 16, 2024

Context:
Most templates are operations themselves. Therefore they should be valid PyTrees, but some are not.

Description of the Change:
This PR adapts the parameters and/or hyperparameters, as well as the methods _flatten and/or _unflatten for a number of parameters.
It adds standard_validity tests to all template test files where assert_valid is not being called on the respective template yet.
Explicit flatten/unflatten tests are removed, because they are contained in assert_valid.

Benefits:
Code quality/self-consistency and consistency of templates with PL functionality.
Test suite quality.

Possible Drawbacks:

Related GitHub Issues:
This helps us go forward with capturing of templates, because primitive_bind_call can be created in a way that is consistent with __init__ and _unflatten.

[sc-63810]

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.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.

pennylane/templates/subroutines/kupccgsd.py Show resolved Hide resolved
pennylane/templates/subroutines/qdrift.py Outdated Show resolved Hide resolved
pennylane/templates/subroutines/qdrift.py Show resolved Hide resolved
pennylane/templates/subroutines/qdrift.py Show resolved Hide resolved
pennylane/templates/subroutines/reflection.py Show resolved Hide resolved
pennylane/templates/subroutines/trotter.py Show resolved Hide resolved
pennylane/templates/subroutines/trotter.py Show resolved Hide resolved
pennylane/templates/subroutines/uccsd.py Show resolved Hide resolved
Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (f5b805b) to head (4bf0ac5).
Report is 264 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5698      +/-   ##
==========================================
- Coverage   99.67%   99.66%   -0.02%     
==========================================
  Files         416      416              
  Lines       38686    38742      +56     
==========================================
+ Hits        38562    38613      +51     
- Misses        124      129       +5     

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

@dwierichs dwierichs added the review-ready 👌 PRs which are ready for review by someone from the core team. label May 16, 2024
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

So I still think there's some issues with QDrift._flatten/ _unflatten. If we bind new parameters for the hamiltonian coefficients, then the decomposition should be appropriately updated. Right now it's not updated. But this might need to be resolved in tandem with the algorithms team.

But other than that issue, this is looking great 🎉

It does bring up the issue of templates being prone to unexpected edge cases. Since it seems like SymbolicOp wasn't actually very helpful with resolving these issues, maybe we could develop a NestedTemplate operator to help with these things. But that is definitely a task for later.

@soranjh soranjh requested a review from Jaybsoni May 21, 2024 18:45
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

👍

Feel free to modify the seed behavior of QDrift so that it's stays None. I approve both options.

Copy link
Contributor

@Jaybsoni Jaybsoni left a comment

Choose a reason for hiding this comment

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

Great work! The updates to assert valid will make it very simple to have one test checking for everything!

pennylane/ops/qubit/arithmetic_ops.py Show resolved Hide resolved
pennylane/templates/subroutines/qdrift.py Show resolved Hide resolved
pennylane/templates/subroutines/qdrift.py Show resolved Hide resolved
tests/templates/test_subroutines/test_qdrift.py Outdated Show resolved Hide resolved
@dwierichs dwierichs enabled auto-merge (squash) May 29, 2024 16:07
@dwierichs dwierichs added merge-ready ✔️ All tests pass and the PR is ready to be merged. and removed review-ready 👌 PRs which are ready for review by someone from the core team. labels May 29, 2024
@dwierichs dwierichs disabled auto-merge May 29, 2024 20:38
@dwierichs dwierichs enabled auto-merge (squash) May 30, 2024 12:26
@dwierichs dwierichs merged commit 856b14e into master May 30, 2024
40 checks passed
@dwierichs dwierichs deleted the valid-templates branch May 30, 2024 12:51
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.

3 participants