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

Disallow taking the adjoint of fractional power operations #5835

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

dwierichs
Copy link
Contributor

@dwierichs dwierichs commented Jun 11, 2024

Context:
The adjoint of an integer power of an operator is the same power of the adjoint of the operator.
For fractional powers, this does not hold because of branch cuts in the power function.

Description of the Change:
This PR explicitly disallows computing the (eager) adjoint of a Pow operator with fractional power, and raises an AdjointUndefinedError in this case.

As a tiny side effect, this PR changes the signature of Identity.pow to be compatible with generalized simplification workflows where the keyword argument z for the power is passed around explicitly (this popped up in a simplification test of Pow)

NB: As usual, a lazy Adjoint(Pow(base, z=0.2)) is still supported, just can't be evaluated/simplified if that would lead to calling the method adjoint of Pow.

Benefits:

Possible Drawbacks:

Related GitHub Issues:
Fixes #5812

[sc-65297]

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.

@EmilianoG-byte EmilianoG-byte self-requested a review June 11, 2024 16:22
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.67%. Comparing base (45f8d4a) to head (cd09001).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5835      +/-   ##
==========================================
- Coverage   99.68%   99.67%   -0.01%     
==========================================
  Files         420      420              
  Lines       40138    39847     -291     
==========================================
- Hits        40010    39717     -293     
- Misses        128      130       +2     

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

Copy link
Collaborator

@EmilianoG-byte EmilianoG-byte left a comment

Choose a reason for hiding this comment

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

This PR looks good! All my comments are related to the case of whole-floats (e.g. z = 2.0) which in my opinion should be treated as integers. But of course, I am open for discussion 😄

pennylane/ops/op_math/pow.py Show resolved Hide resolved
pennylane/ops/op_math/pow.py Show resolved Hide resolved
tests/ops/op_math/test_pow_op.py Show resolved Hide resolved
tests/ops/op_math/test_pow_op.py Show resolved Hide resolved
tests/ops/op_math/test_pow_op.py Show resolved Hide resolved
@albi3ro albi3ro self-requested a review June 11, 2024 19:03
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.

We can handle exponents like z=1.0 at some later point. This fixes the issue.

@EmilianoG-byte EmilianoG-byte self-requested a review June 12, 2024 16:10
Copy link
Collaborator

@EmilianoG-byte EmilianoG-byte left a comment

Choose a reason for hiding this comment

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

Based on our discussion, I also agree the current changes are fine for now 😃. Thank you for the discussion!

@dwierichs dwierichs enabled auto-merge (squash) June 12, 2024 20:57
@dwierichs dwierichs added the merge-ready ✔️ All tests pass and the PR is ready to be merged. label Jun 12, 2024
@dwierichs dwierichs merged commit 3771f24 into master Jun 12, 2024
41 checks passed
@dwierichs dwierichs deleted the adjoint-pow branch June 12, 2024 23:38
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.

[BUG] qml.Adjoint and qml.Pow do not commute when exponent is fractional
3 participants