-
Notifications
You must be signed in to change notification settings - Fork 167
Add inverse to QFT implementation #705
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
base: devel
Are you sure you want to change the base?
Conversation
quest/include/operations.h
Outdated
| * | ||
| * @formulae | ||
| * | ||
| * Let @f$ N @f$ = @p numTargets, the @f$ N @f$ qubit Quantum Fourier Transform maps each |
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.
Let -> Letting
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.
Updated in 3b07c0c.
quest/include/operations.h
Outdated
| * @formulae | ||
| * | ||
| * Let @f$ N @f$ = @p numTargets, the @f$ N @f$ qubit Quantum Fourier Transform maps each | ||
| * computational basis state belonging to the targeted qubits, @f$ \ket{j} @f$, according to |
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.
belonging to -> of
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.
Updated in 3b07c0c.
| * @param[in] inverse whether to apply the inverse QFT or forward QFT | ||
| * @throws @validationerror | ||
| * - if @p qureg is uninitialised. | ||
| * - if @p targets are invalid qubit indices. |
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.
add:
if @p targets are not unique.
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.
Updated in 3b07c0c.
| * - applyFullQuantumFourierTransform() | ||
| * @author Vasco Ferreira | ||
| */ | ||
| void applyQuantumFourierTransform(Qureg qureg, int* targets, int numTargets, bool inverse); |
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.
Technically, type bool is only gauranteed to be defined in C when including the <stdbool.h> header. That header is in the standard since C99 however. @otbrown thoughts on this? This is the first bool in the QuEST API (ignoring measurement outcomes passed as int). Should we make inverse an int and validate it to be ==0 or ==1 (like measurement outcomes), or just include the header?
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.
Happy to update this, just a note that PR #702 also included a bool (and the <stdbool.h> header for C), so if we do end up going with the int and validation route, it might make sense to update that too.
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.
Good catch @TysonRayJones!
My thoughts are that:
- We are already using the C11 standard, so introducing
stdbool.hdoesn't introduce any build issues or push our requirements. - Practically, bool vs int makes very little difference as they are somewhat interchangeable in C, however bool has a semantic benefit, as it better expresses intent.
So in my opinion we should include stdbool.h and use bool here.
Howdy, I hope your holidays and the start of the new year have been great!
While the refactor of the QFT implementation in #604 remains in the backlog, I think it would still prove useful to have an inverse QFT for completeness. Since I needed this for another project and it is a fairly minor change I figured it might be worth tidying it and submitting a PR.
Main changes:
inverseparameter, which is a breaking change, but I think it makes sense to keep the QFT and inverse together. Happy to split it if you disagree.Cheers!