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 19 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.
This class appears in the Section "Generalized Gates", unlike QFT which appears in the section "Basis Change Circuits".
Is it deliberate?
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! It's a gate but not a "basic gate", so it's a "generalized gate". Are you proposing to restructure the library of generalized gates, grouping the generalized gates by "their purpose"?
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.
I'm actually not sure why QFT appears separately than the other generalized gates
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.
Generalized gates were thought to be "extensions" of the standard gates, such as multi-Pauli-gates or uniformly controlled gates. But by now there's a lot of other objects there that we don't yet have a better category for, like Isometry or Permutation. It also doesn't really matter as we allow (and encourage) import from
qiskit.circuit.library
, but I'd keepQFTGate
it inbasis_change
for the time being.We probably won't put all gates into
generalized_gates
since that wouldn't be any sorting really, hopefully we'll be able to clean it up a bit in the restructure.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.
I think that maybe all the (multi/uniformly) controlled gates deserve a special category (but that's not relevant to this PR)
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. I have moved the
QFTGate
tobasis_change
, the same file that contains theQFT
circuit.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.