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

Update 4-term parameter shift coefficients #1206

Merged
merged 6 commits into from
Apr 14, 2021
Merged

Update 4-term parameter shift coefficients #1206

merged 6 commits into from
Apr 14, 2021

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Apr 13, 2021

Description of the Change: Updates the four-term parameter-shift rule to use coefficients that minimize the variance as per https://arxiv.org/abs/2104.05695

Benefits: More optimal choice of coefficients

Possible Drawbacks: n/a

Related GitHub Issues: n/a

@github-actions
Copy link
Contributor

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

@josh146 josh146 requested a review from antalszava April 13, 2021 11:42
@codecov
Copy link

codecov bot commented Apr 13, 2021

Codecov Report

Merging #1206 (6530972) into master (e17dc37) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1206   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files         145      145           
  Lines       10985    10985           
=======================================
  Hits        10779    10779           
  Misses        206      206           
Impacted Files Coverage Δ
pennylane/ops/qubit.py 98.29% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e17dc37...6530972. Read the comment docs.

Copy link
Contributor

@antalszava antalszava 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! 💯 🎊 None of the suggestions are blockers.

pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
Co-authored-by: antalszava <antalszava@gmail.com>
@dwierichs
Copy link
Contributor

This looks good to me, up to a tiny documentation issue: The latest change of the sign in the CRX docstring would need to be applied to CRY, CRZ and CRot as well, wouldn't it?

pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
@josh146
Copy link
Member Author

josh146 commented Apr 14, 2021

Thanks @dwierichs, have just made them all consistent :)

@josh146 josh146 merged commit 2450d8c into master Apr 14, 2021
@josh146 josh146 deleted the 4-term-shift branch April 14, 2021 08:29
@dwierichs
Copy link
Contributor

Great, thanks for carrying out this PR! :)

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.

3 participants