-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 9858055267Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
I had a discussion with @ShellyGarion and a discussion with @Cryoris on whether In the original QFT synthesis algorithm for all-to-all connectivity, the synthesized QFT circuit has a layer of swap gates at the end. These swap gates correspond to a very specific reversal permutation. Other circuits that use QFT as a building block, in many cases use this special knowledge and avoid explicitly implementing the reversal implementation with swaps when the reversal can be achieved in a different way. For instance, for circuits that have both QFT and its inverse, the two reversal permutations cancel out and hence do not need to be implemented at all. (Note that the name Since the goal is to make sure that the QFT-based application circuits have count/depth as small as possible, we should investigate if in all cases the transpiler is able to automatically optimize away the reversal permutations if they were to appear in the circuits. If this is the case, then we can have the canonical QFT gate (and we do not need to change the implementation in this PR). If this is not the case, then we may want to add to a QFT gate the argument |
imo, a In other words, if it's going to be a With the advent of |
Jake, thanks for the feedback.
I completely agree with this, and this is how it's implemented right now. Though I still have not checked if we get the same or worse results when we substitute QFT circuits (with Note: to avoid possible confusion, for the QFT circuit to faithfully implement the QFT matrix using the default implementation in In theory I agree with you that things like |
An update on how QFT-subcircuits are used within Qiskit's circuit library (thanks to @ShellyGarion and @Cryoris for all the help). We have 4 circuits that use QFT as a building block: In each of these cases, the main circuit uses a sneaky optimization of using not the proper QFT, but the QFT with the argument
The optimization (removing the final reversal permutation and modifying the rest of the circuit accordingly) makes sense for he default QFT synthesis (targeting all-to-all connectivity), as it introduces the layer of swaps (aka the reversal permutation) at the end of the circuit, so the "QFT-with-reversal" is the simple circuit without this layer of SWAPs. Note however that when transpiling for general architectures, routing may insert many additional SWAPs. But the optimization no longer makes sense for other QFT synthesis algorithms, such as the one targeting the linear-nearest neighbor connectivity, since it does not introduce a reversal permutation at the end of the circuit. Hence, for the In addition, it is easy to check that if in the The bottom line of the discussion is that we don't really need to have the "reversed" version of QFT for the transpiler to produce optimal circuits, as long as we can properly use the Hence it probably makes sense to have the new Though, some thought is required on how to properly combine the |
I agree with the final goal of having this be an |
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 Sasha, this is looking good. For the purposes of ElideSwaps
and things like that, maybe a route along the lines of letting synthesis routines synthesise up to a permutation, then outputting a permutation gate as well? That way ElideSwaps
can just pick that up and remove it, or the behaviour can generally be controlled.
num_qubits = self.num_qubits | ||
mat = np.empty((2**num_qubits, 2**num_qubits), dtype=dtype) | ||
for i in range(2**num_qubits): | ||
i_index = int(bin(i)[2:].zfill(num_qubits), 2) | ||
for j in range(i, 2**num_qubits): | ||
entry = np.exp(2 * np.pi * 1j * i * j / 2**num_qubits) / 2 ** (num_qubits / 2) | ||
j_index = int(bin(j)[2:].zfill(num_qubits), 2) | ||
mat[i_index, j_index] = entry | ||
if i != j: | ||
mat[j_index, i_index] = entry | ||
return mat |
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
def qft(n):
pows = np.arange(2**n)
return np.exp(2j * np.pi / n * np.outer(pows, pows)) * (0.5 ** (n/2))
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.
def _basic_decomposition(self): | ||
"""Provide a specific decomposition of the QFT gate into a quantum circuit. | ||
|
||
Returns: | ||
QuantumCircuit: A circuit implementing the evolution. | ||
""" | ||
from qiskit.synthesis.qft import synth_qft_full | ||
|
||
decomposition = synth_qft_full(num_qubits=self.num_qubits) | ||
return decomposition | ||
|
||
def _define(self): | ||
"""Populate self.definition with a specific decomposition of the gate. | ||
This is used for constructing Operators from QftGates, creating qasm | ||
representations and more. | ||
""" | ||
self.definition = self._basic_decomposition() |
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.
Nice contribution @alexanderivrii. I have a few comments and suggestions.
-
Do we plan to deprecate
QFT
in favor ofQFTGate
in Qiskit 2.0 ? -
You mentioned the use-case of adders and circuits containing QFT and QFT-inverse, do you have some test for it? or will it appear in a future PR?
It is impossible ti implement the QFT approximately by ignoring | ||
controlled-phase rotations with the angle is beneath a threshold. This is discussed | ||
in more detail in https://arxiv.org/abs/quant-ph/9601018 or | ||
https://arxiv.org/abs/quant-ph/0403071. |
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.
After a long delay, I have hopefully addressed all of the review comments; this is ready for review once again. Now we have the Deprecating |
from qiskit.circuit.quantumcircuit import Gate | ||
|
||
|
||
class QFTGate(Gate): |
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 keep QFTGate
it in basis_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
to basis_change
, the same file that contains the QFT
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.
This PR looks very good to me. I only have a few questions about the documentation.
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 looks great! Just some minor comments below.
from qiskit.circuit.quantumcircuit import Gate | ||
|
||
|
||
class QFTGate(Gate): |
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 keep QFTGate
it in basis_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.
It is impossible ti implement the QFT approximately by ignoring | ||
controlled-phase rotations with the angle is beneath a threshold. This is discussed | ||
in more detail in https://arxiv.org/abs/quant-ph/9601018 or | ||
https://arxiv.org/abs/quant-ph/0403071. |
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.
LGTM! Thanks @alexanderivrii !
…iskit#11463) * initial commit * release notes * fixing synthesis plugin options * finalize merge conflicts * fixing default option values for qft plugins' * additional tests for qft plugins * renaming QftGate to QFTGate * Also renaming Qft to QFT in synthesis method names * appplying Jake's suggestion from code review * inlining _basic_definition into _define * docstring improvements * more docstring improvements * renaming do_swaps to reverse_qubits in the new code * typos * adding synth_qft_full to __init__ * round of suggestions from code review * another round of code review suggestions * fixes * also adding QFTGate plugins to the docs
Summary
A new "high-level" gate
QFTGate
allows to natively add QFTs on a quantum circuit and, in particular, to defer their synthesis to the transpiler. The gates will be synthesized by theHighLevelSynthesis
transpiler pass, using one of several available plugins.A new function
synth_qft_full(num_qubits, do_swaps, approximation_degree, insert_barriers, inverse, name)
allows to synthesize a QFT circuit with the given number of qubits. For now the implementation is taken directly from the_build
method of the existingQFT
(blueprint) circuit class. The name of the method reflects that the synthesized circuit requires full connectivity. Note that another QFT synthesis method has been recently added in #11236, with the synthesized circuit following only linear (aka nearest-neighbor) connectivity.The two methods above are wrapped into HighLevelSynthesis plugins
QFTSynthesisFull
andQFTSynthesisLine
respectively. The default synthesis is set toQFTSynthesisFull
, thus fully coinciding with the circuits produced using the existingQFT
circuit class.In a follow-up PR I am going to replace the construction of QFT circuits by QFT gates whenever possible, such as for example when constructing QFT-based adders and multipliers.
Details and Comments
The constructor in the original
QFT
circuit class includes argumentsnum_qubits
,approximation_degree
,do_swaps
,inverse
,insert_barriers
andname
. Hereapproximation_degree
allows to ignore small controlled-phase rotations, this leading to an approximate but more efficient circuit, anddo_swaps
allows to ignore SWAP gates at the end of the circuit. An important decision is which of the arguments above should be a part of the definition of aQFTGate
, and which should be parameters to its synthesis method. In this PR onlynum_qubits
is part of theQFTGate
, while the rest are parameters to the synthesis method. For instance, I strongly believe that whether to include barriers in the synthesized circuit and which name to assign to the synthesized circuit have nothing to do with the definition of theQFTGate
. I am also inclined to think thatapproximation_degree
anddo_swaps
should be synthesis parameters, i.e. instead of talking about an approximate swap-reduced QFT-gate, we are talking about a QFT-gate that can be synthesized ignoring small control-phase rotations and final swaps, especially that deciding which gates in the synthesized circuit can be ignored heavily depends on the synthesis method itself. What, however, is missing is incorporating the knowledge that in some cases in the bigger circuit we haveinverse QFT-gate -- some gate U -- QFT-gate
is equivalent toinverse swap-reduced QFT-gate -- some gate U -- swap-reduced QFT-gate
, that is it is safe to drop swaps for both QFT gates. This is important as it leads to a smaller overall circuit, however for this to be correct both QFT gates should be synthesized in the same way. And we currently don't have high-level-synthesis API to ensure this. Alternatively we could adddo_swaps
to the gate definition, however I don't really like this solution, and it also does not solve the problem: in theoryHighLevelSynthesis
can choose different QFT-synthesis algorithms for both gates, again leading to incorrect results when ignoring swap gates. Thoughts and suggestions are welcome.