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

Set grad_method=None for ControlledSequence, Reflection, AmplitudeAmplification, and Qubitization. #5806

Merged
merged 7 commits into from
Jun 6, 2024

Conversation

astralcai
Copy link
Contributor

Context:
Templates that are not actually supported by parameter_shift should have grad_method=None so that they are decomposed by _expand_transform_param_shift

Description of the Change:

  1. Adds the data of components of the templates to the data of the templates such that trainable parameters are tracked
  2. Adds grad_method=None for ControlledSequence, Reflection, AmplitudeAmplification, and Qubitization.

Related GitHub Issues:
Fixes #5802
[sc-64967]

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (38b3e74) to head (ba16407).
Report is 250 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5806      +/-   ##
==========================================
- Coverage   99.66%   99.66%   -0.01%     
==========================================
  Files         415      415              
  Lines       39601    39309     -292     
==========================================
- Hits        39469    39176     -293     
- Misses        132      133       +1     

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

@astralcai astralcai marked this pull request as ready for review June 5, 2024 20:44
@trbromley
Copy link
Contributor

CC @KetpuntoG

@KetpuntoG
Copy link
Contributor

CC @KetpuntoG

ControlledSequence, AmplitudeAmplification and Reflection from a theoretical point of view should be differentiable with parameter shift rule. Is this PR due to a PL limitation?

@astralcai
Copy link
Contributor Author

astralcai commented Jun 6, 2024

CC @KetpuntoG

ControlledSequence, AmplitudeAmplification and Reflection from a theoretical point of view should be differentiable with parameter shift rule. Is this PR due to a PL limitation?

I've done some investigation but haven't found out why directly taking parameter shift on the templates return incorrect results. See #5802 for more details.

@albi3ro
Copy link
Contributor

albi3ro commented Jun 6, 2024

CC @KetpuntoG

ControlledSequence, AmplitudeAmplification and Reflection from a theoretical point of view should be differentiable with parameter shift rule. Is this PR due to a PL limitation?

To clarify, this doesn't say "don't use parameter shift with these templates". It is "decompose before applying parameter shift".

Theoretically, we could develop parameter shift rules for the templates and add them to the grad_recipe. But that seems like a lot of unnecessary work when we can just decompose them. Since this is fixing a bug where pennylane is giving wrong answers and blocking a P0, we should proceed with this fix for now. But if you are interested in developing a customized parameter shift recipe for the templates, I am willing to support.

Would there be a benefit to being able to natively apply parameter shift to these templates instead of to their decomposed form? For some templates, like CommutingEvolution, it might lead to fewer total parameter shift tapes.

@KetpuntoG
Copy link
Contributor

CC @KetpuntoG

ControlledSequence, AmplitudeAmplification and Reflection from a theoretical point of view should be differentiable with parameter shift rule. Is this PR due to a PL limitation?

To clarify, this doesn't say "don't use parameter shift with these templates". It is "decompose before applying parameter shift".

Theoretically, we could develop parameter shift rules for the templates and add them to the grad_recipe. But that seems like a lot of unnecessary work when we can just decompose them. Since this is fixing a bug where pennylane is giving wrong answers and blocking a P0, we should proceed with this fix for now. But if you are interested in developing a customized parameter shift recipe for the templates, I am willing to support.

Would there be a benefit to being able to natively apply parameter shift to these templates instead of to their decomposed form? For some templates, like CommutingEvolution, it might lead to fewer total parameter shift tapes.

Ah okey! That's perfect: first decompose, then shift-rule
Thanks for the clarification

Copy link
Contributor

@KetpuntoG KetpuntoG left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good to me :)

Copy link
Contributor

@dwierichs dwierichs 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 to me! 🎉 Thanks!

@astralcai astralcai enabled auto-merge (squash) June 6, 2024 18:08
@astralcai astralcai merged commit 22da9a0 into master Jun 6, 2024
40 checks passed
@astralcai astralcai deleted the template-grads branch June 6, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Some templates not properly differentiated with parameter shift on the legacy device
5 participants