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

Calculate complementary polynomial for QSP using flip convolution. #930

Merged
merged 66 commits into from
May 31, 2024

Conversation

Epsilon1024
Copy link
Contributor

Calculate complementary polynomial using Eq. 60 of the paper

@Epsilon1024
Copy link
Contributor Author

There are two small followups I will be adding after this PR is submitted:

  1. This version does not normalize P before calculating Q. (From my understanding the first version of the default QSP did not normalize P, but this function was added later). I will be adding the normalization in a follow-up PR (that is nearly finished).

  2. The main challenge that I faced in this PR was setting a high tolerance for the case with all real polynomials. I found the method in this PR performs comparatively to that in the code provided by the authors of the paper. I will write out my findings in a notebook to go along with this.

@Epsilon1024 Epsilon1024 marked this pull request as ready for review May 20, 2024 21:48
Copy link
Contributor

@anurudhp anurudhp 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! Mostly docstring+type-hint nits. Also please fix the rng comment, best practice is to use np.random.Generator.

qualtran/bloqs/qsp/fast_qsp.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp_test.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp_test.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp_test.py Outdated Show resolved Hide resolved
qualtran/bloqs/qsp/fast_qsp_test.py Outdated Show resolved Hide resolved
@Epsilon1024
Copy link
Contributor Author

As a note: I ran some checks to see if normalizing P before running the tests on fast_complementary_polynomial would give us more accurate results. From my checks, it performs slightly worse than not being normalized. This tells me that the precision problem is not related to normalization.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

@anurudhp @Epsilon1024 Can we merge this and continue improvements in follow-up PRs? Maybe open an issue to track the potential improvements?

@anurudhp
Copy link
Contributor

@tanujkhattar good to go % hardcoded random seed

Copy link
Contributor

@anurudhp anurudhp left a comment

Choose a reason for hiding this comment

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

LGTM!

qualtran/bloqs/qsp/fast_qsp.py Outdated Show resolved Hide resolved
@tanujkhattar tanujkhattar enabled auto-merge (squash) May 31, 2024 19:21
Co-authored-by: Anurudh Peduri <[email protected]>
@tanujkhattar tanujkhattar merged commit 267a05f into quantumlib:main May 31, 2024
7 checks passed
@anurudhp anurudhp mentioned this pull request Aug 1, 2024
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