Skip to content

Conversation

@Epsilon1024
Copy link
Contributor

Added fft_qsp module to address Issue 1076

This fixes the error issues found by fast_complementary_polynomial by using the algorithm described in Berntson and Sunderhauf (2024) (Algorithm 2 based off the code in Figure 1). This method takes the target precision as an input parameter and automatically determines the number of FFT modes needed to guarantee the given precision.

@anurudhp
Copy link
Contributor

@Epsilon1024 could you rebase on main and squash all the commits so far? there's a lot of legacy commits, it's hard to keep track of new changes to review.

@tanujkhattar
Copy link
Collaborator

@anurudhp You can just go to files changed and it shows only two new files?

image

@anurudhp
Copy link
Contributor

Right that's true. I meant more for reviewing incremental changes, but I guess I can just look from the last two commits, so no issue.

@christoph-riverlane
Copy link

christoph-riverlane commented Jun 18, 2024

Hi all,
thanks Tanuj for tagging me, and thanks for the interest in our algorithm and paper. Two notes:

  • Our proofs require N even. However, the algorithm appears to work also for odd N. (In fact, for our numerical results, we have used both odd and even values of N. We'll add a comment on this to the paper.) I think our proofs can also be extended to odd N.

  • The values for N from the proof are not tight: We found that sometimes in practice N can be sufficient that are significantly smaller, see eg the values of N in Fig 2. Therefore, we had envisioned that in practice, a search procedure might be used, in which the algorithm is run repeatedly with doubled N until the desired accuracy is reached. (We'll add a comment on this to the paper)

Cheers, Christoph

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.

Very nice. Left a first round of comments.

Could you investigate what the best tolerance values are for the current implementation? I suspect they are much better than the current test parameters.

Untitled.ipynb Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

this notebook seems quite useful, perhaps name it appropriately and move it to qualtran/bloqs/qsp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

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! left some comments on docstring fixes, otherwise lgtm

cc @tanujkhattar

Copy link
Contributor

Choose a reason for hiding this comment

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

please delete this file

@anurudhp anurudhp enabled auto-merge (squash) August 1, 2024 18:11
@anurudhp anurudhp merged commit d49e6e0 into quantumlib:main 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.

4 participants