-
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
High-level-synthesis for permutations #9157
Conversation
…vity and has depth guaranteed by the algorithm
Co-authored-by: Nir Gavrielov <nirgavrielov@gmail.com>
…kit-terra into permutations-over-lnn
…nnectivity; including tests and plugin
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!
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 the code itself LGTM, the Gate
based class and synthesis routines look fine (although I scanned it kind of quickly). My bigger concern here though is around backwards compatibility. Making a breaking change to the Permutation
class like this is a bit problematic and will likely break any existing users of the class. For this PR I feel we should introduce the Gate
based permutation as a new class and update the circuit based class to internally just be a single PermutationGate
.
Then to make this transition I think we'll have to follow this basic playbook:
- This release add new
Gate
based permutation class (PermutationGate
is the class name I can think of, but any other ideas are welcome) - Next release deprecate
Permutation
(QuantumCircuit
based class) - 2 releases later remove
Permutation
- 3 releases later add alias of
PermutationGate
to oldPermutation
name - 4 releases later deprecate
PermutationGate
- 6 releases later remove
PermutationGate
in favor ofGate
basedPermutation
class
4-6 are optional and only really needed if we strongly desire the end result of Permutation
being a Gate
(and not having a new name)
from qiskit.circuit.exceptions import CircuitError | ||
|
||
|
||
class Permutation(QuantumCircuit): | ||
"""An n_qubit circuit that permutes qubits.""" | ||
class Permutation(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.
I'm a bit concerned about making this change from a backwards compatibility PoV in a single release. While I agree with this being a Gate
subclass instead of a QuantumCircuit
is where we want to be I'm worried about any users of this class today that may be using it with a circuit as is. For example the docs here show a good example of this with Permutation(...).draw()
, but there are others I can think of the top of my head would be something like: Permutation(...).compose(sub_circuit)
or Permutation.measure_all()
all of which were previously completely valid which would be broken by this change.
I'm thinking we need to figure out a better way to manage this migration which leaves the QuantumCircuit
based Permutation
class in place as is for now but establishes a new Gate
based class as a standalone thing (like PermutationGate
or something) in this PR and maybe update this QuantumCircuit
based one to just wrap the new Gate
class. Then in the next release we can deprecate the QuantumCircuit
version and tell people to use the gate directly.
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, this is a very good point. I keep relying on qiskit-terra tests to make sure that nothing gets broken, yet I keep forgetting that there is also a lot of other code outside of qiskit-terra. The example with Permutation.measure_all()
clearly shows why we need to keep the old functionality. It should be quite straightforward for me to keep the old implementation and to call the new implementation differently, in this way this development no longer changes any existing behavior, it only adds new things.
I have already started doing changes, but here are a couple of preliminary thoughts. The name PermutationGate
for the new class is good. For objects of type PermutationGate
we can still have op.name = "permutation"
, since there is no danger to confuse permutation circuits and permutation gates (and in any case the name assigned to a permutation circuit is never "pemutation"
but rather something like "permutation_[2,4,3,0,1]"
). In particular, we can keep using names like permutation.kms
in entry_points
to specify permutation synthesis algorithms for PermutationGate
objects.
The suggested deprecation flow also allows to avoid passing num_qubits
and seed
to the initializer of the PermutationGate
, making the code a bit cleaner. There is a function np.random.permutation
that returns a random permutation pattern, from which a PermutationGate
can be constructed.
…kward compatibility, and naming the new permutation gate class as PermutationGate
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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 LGTM now, thanks for doing this and making all the updates from review.
Summary
This PR is based on top of #9082. First, it introduces
PermutationGate
for representing permutation logic, which is of typeGate
rather thanQuantumCircuit
. The new way of storing permutation logic avoids synthesizing a permutation circuit when thePermutation
gate is declared, delaying the actual synthesis to transpiler. It also allows to easily choose between several different algorithms for synthesizing permutations, which are available as high-level-synthesis permutation plugins.Then it adds 3 different high-level-synthesis plugins for permutations. The
BasicSynthesisPermutation
plugin applies to fully-connected architectures and is based on sorting. This is the previously used algorithm for constructing quantum circuits for permutations. TheACGSynthesisPermutation
plugin also applies to fully-connected architectures but is based on the Alon, Chung, Graham method. It synthesizes any permutation in depth 2 (measured in terms of SWAPs). TheKMSSynthesisPermutation
plugin applies to linear nearest-neighbor architectures and corresponds to the recently added Kutin, Moulton, Smithline method.The
PermutationGate
has a new method__array__
that returns a unitary matrix for a permutation, in particular allowing to create unitary matrices from circuits containing permutation gates. This might be immediately useful for #8597 (see #8597 (comment)).In the future, it would also make sense to add the permutation synthesis plugin based on
AproximateTokenSwapper
.Example:
produces
Note that "kms" method adheres to LNN architecture. A quick experiment on more random permutations shows that the new "acq" method seems to produce the same number of gates as our previous method, but has better depth (which is always at most 2 compared to 5 in this example). However, I would like to add support for coupling map and run more experiments before changing the default.