Skip to content

Conversation

@NoureldinYosri
Copy link
Contributor

@NoureldinYosri NoureldinYosri commented Sep 9, 2024

Originall created in #1371. I had to recreate the PR because of issues with my clone of Qualtran.

I addressed the comments on the original PR and switched from QROM to QROAMClean which leads to a better Toffoli complexity than in table 8 in Litinski https://arxiv.org/abs/2306.08585.

This implementation follows appendix C4 in Gouzien https://arxiv.org/abs/2302.06639 and has Toffoli complexity of $2.25n^2+8.25n-1$ which is better than the $2.25n^2+9n$ in table 8 in Litinski https://arxiv.org/abs/2306.08585

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

see comments

target += Tm
target >>= self.window_size
qrom_indices = (qrom_indices << self.window_size) | m
return target, qrom_indices
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is now duplicate code, right? See my other comment about how we can make the classical action more readable / useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no .. this is part of the classical action function for DirtyOutOfPlaceMontgomeryModMul

Copy link
Collaborator

Choose a reason for hiding this comment

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

this code is nearly identical to SingleWindowModMul.on_classical_vals. Can you explain the distinction (if any) between the two, preferably in code comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main difference is that SingleWindowModMul.on_classical_vals operates on bitarrays. this methods operates on ints.

added a comment.

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

can you please address my remaining new comments, then lgtm

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