Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding QFT gate to natively reason about Quantum Fourier Transforms #11463
Adding QFT gate to natively reason about Quantum Fourier Transforms #11463
Changes from 8 commits
71efe1c
3705ba4
68c2ccf
c835e20
425a6d9
ef45120
09086e6
f1d4215
5d8be05
eac4ed6
cac843b
5acfc3f
0b605bc
6106c4a
a76fbe7
0d06898
71860bf
d496f08
2822d5c
6feb4fc
a0ea5ba
b230c58
8806c27
4ae4c9e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
is probably a fair bit faster than this. I think there might be some nicer
np.power.outer
tricks that might be faster, but it probably doesn't matter too much at the scale of matrices that we can generate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool! Done in 6106c4a. On top of this, I have utterly no idea why I was converting an integer to its binary representation and back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor nitpicking, but is there a need to have a separate
_basic_decomposition
function? Can we inline it into_define
(which largely implies that it's a basic definition)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done in a76fbe7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add them as references?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am slightly inclined to keep it a bit less formal: IMHO, a good reference section would first cite a paper defining a QFT operation, then cite papers that introduce different synthesis methods for QFT, and only then papers that discuss approximation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would also prefer the references written out, because of (1) consistency with other code and (2) being able to read authors & title w/o clicking the link (but maybe that's just me being lazy 😛 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to popular demand, the references are now written out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I think that you should still remove line 912
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, forgot to delete this line, done now.