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
Add ElidePermutations transpiler pass #9523
Add ElidePermutations transpiler pass #9523
Changes from 47 commits
e90bdc7
caa6a73
e757164
438e0a9
8f699e8
8dfd4f7
3b28f06
ec8af2d
599bc67
ad3697c
1d30a65
2f13d9d
5688293
955dec3
a364c08
695f35b
39fd3bb
1ea19ee
95e7c11
1d37611
b27e52e
9ece52c
0f1df15
1aba9fc
723fd42
11b4156
8ff19a6
40f4c4e
87ce691
c1fd952
33e867d
6f0f679
f698977
31b23f2
32cc904
960f592
e978874
b3c6252
80cd91a
cc621f1
33ac77f
ddfde6a
aaa3365
15e9809
2b203e1
ba49503
bb80234
5888c70
07c2966
e270aa8
f5b6778
5cbc67d
e755bcd
28263bf
3c967c3
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.
I'd like to review properly once I'm back at work on Tuesday, but just as an immediate note: please can we consider not using
Layout
here? As both its name and the top PR comment says, thevirtual_permutation
is a permutation of virtual-to-virtual, whereas theLayout
class is explicitly a mapping of virtual to physical qubits (i.e.Layout
is a map from one Hilbert space to a different Hilbert space, whereas this is an operator within the same Hilbert space). Theidx
here are not physical qubits, they're just virtual-qubit indices, and I think we ought to separate the representations, especially given how confused we keep getting aboutLayout
.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.
By way of alternative, I mean I'd prefer just to store an int list of the permutation, which is how we store permutations in all other places (like
PermutationGate
).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.
Jake, I very much agree with you that using the word layout to represent anything else than virtual-to-physical is confusing. For instance, it took me a lot of time and an offline discussion with Matthew to understand that "final_layout" stands for the physical-to-physical permutation added during the routing stage. Having said that, I feel that at this point having both virtual-to-physical and physical-to-physical maps called layouts and internally represented using the
Layout
class, while having the virtual-to-virtual map being something else would be even more confusing. So not my favorite choice, but I am arguing for keeping the virtual-to-virtual map as aLayout
as well.It might be simplest to think of the word layout as of some mapping$f: A \rightarrow B$ , where $A$ and $B$ can be anything. One nice thing is that you can easily compose mappings, e.g. composing the mappings $f:A\rightarrow B$ and $g:B\rightarrow C$ produces the mapping $g\circ f:A\rightarrow C$ . The functionality to compose
Layout
s was recently implemented in PR #11399, and this simplifies the code that deals withLayout
s by a lot.In addition, we are now talking about multiple passes that can modify the virtual-to-virtual permutation: e.g., this
ElidePermutations
pass and the newStarPreRouting
pass. To keep track of the combined permutation we can simply composeLayouts
.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 storing
final_layout
as aLayout
was a mistake, and that was borne out in how nobody was able to use/understandTranspileLayout
at first (both us - see all the issues we had in implementingOperator.from_circuit
- and users) and we had to completely remake the public interface in #10835.In an ideal world, we'd walk back
final_layout
but we only didn't in #10835 because it would have been a breaking change so we just left it. I'd really prefer that we didn't expand that bad situation. I wouldn't say we represent physical-to-physical permutations everywhere as Layouts at all, we just do it in this one place that imo we already recognised as a mistake.I'm also not super wild that we merged #11399 - to me, that PR has a mistake even in its top comment where is even suggests that a
Layout
can have an inverse. A permutation can have an inverse, but a layout can't - a Layout doesn't have a "forwards" and "backwards", it has a "virtual" side and a "physical" side, and every part of the data structure, public method and argument typing says that.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 am fine with any decision but I do find it so much easier to think about all of this (including how to guarantee that unitary equivalence is preserved) when I think of a layout as of an invertible mapping. For better or for worse, even the "layout" (in the property set terminology) aka the "initial_layout" (in
TranspileLayout
terminilogy) is an invertible mapping from ancilla-expanded virtual qubits to physical qubits. I would argue that this is also a mistake (I mean the ancilla-expanded part of this), but this is another thing that we are stuck with for now. But if we do work with invertible mappings, having the inverse mapping is very natural and really helps writing shorter cleaner code. At this point I feel it would be significantly more confusing if we did not repeat the same mistake once again. :)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.
Oh, also, in case this is the part we're missing each other on: I wouldn't expect to be able to directly compose the result of the permutation that I'm talkin about with the permutation induced by a routing pass, because we're expecting
ElideSwaps
to run on a virtual circuit, and routing runs on a physical circuit, so the permutations apply to two different Hilbert spaces.In my version of the transpiler pipeline,
ApplyLayout
would convert the virtual permutation to a physical permutation (likewiseFullAncillaAllocation
would pad it to fill width, etc, just like these passes will do to operators when/if we transpiler EstimatorPubs), and then they'd be directly composable, because only then would they be acting on the same spaces. There would be no need forFinalizeLayout
.Layout
keeps confusing the matter because it makes us think the "virtual qubits" are always the same thing in layouts at any transpiler pass, but the misuse ofLayout
and the existence ofFullAncillaAllocatiom
makes that not at all true. The two*_virtual_layout
methods ofTranspileLayout
do produce layouts with the same virtual qubits, which is why they actually directly interact cleanly.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.
So, would it be possible to have the circuit's
TranspileLayout
instance available to every transpiler pass (and not just at the end of the run) with every pass being able to update the fields of theTranspileLayout
and not the "layout" and "final_layout" attributes of the property set? (And if these are needed for backward compatibility, then somehow retrieve them from theTranspileLayout
instead)?Oh yeah the whole reason to invent the
FinalizeTranspiler
pass was thatSetLayout
andApplyLayout
do not always run (iirc they do not run when no coupling map or target is specified). I thought there is a deep reason why we might want to skip running these passes, but if not, we could indeed delegate the job of lifting the permutation of virtual qubits to a permutation of physical qubits to theApplyLayout
pass (which was actually Matthew's original suggestion).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.
No,
TranspileLayout
will never be available to any transpiler pass - it's not created untilPassManager.run
is converting the finalDAGCircuit
back to aQuantumCircuit
. We can track whatever we like in order to build it at the end, though.If
ApplyLayout
never runs, then the circuit never swaps from being virtual to physical; the output remains a virtual circuit. That's expected to me -ApplyLayout
's job is to convert virtual into physical. I don't think that's a problem here - it would just mean that the only method that ought to do anything onTranspileLayout
should berouting_permutation
(which I guess is slightly misnamed now - it's not only routing); for all the others, there is noLayout
, so{initial,final}_{virtual,index}_layout()
would be whatever they are when the circuit wasn't laid out.I was mostly saving this for tomorrow, but since I'm here, let me sketch out how I'd make sense of permutations and the responsibility splits all the way through the transpiler to avoid needing extra stuff on
Layout
. I think that this actually along the lines of what Matt was originally intending withfinal_layout
, except I think the use of theLayout
class totally confused things and then we got all lost in the weeds.ElideSwaps
and all the routing passes, but not the layout passes) can be thought of as taking a circuitc
to a new circuitc2
that's implicitly followed by aPermutationGate(perm2)
acting ondag.qubits
, so thatmatrix(c) == matrix(PermutationGate(perm2)) @ matrix(c2)
.perm2
in the above isn't aLayout
, it's directly linked to indices into whateverdag.qubits
is at the current point in the pipeline. We storeperm2
inPropertySet["final_permutation"]
(note: we could equally calculate and storePermutationGate(perm2).inverse().pattern
, i.e. store either the additive or the subtractive part. We can choose whatever convention is more convenient for composition.)c2
that elides more swaps and so mapsc2
to(c3, perm3)
, the full action of the transpilation pipeline is such thatmatrix(c) == matrix(PermutationGate(perm2)) @ matrix(PermutationGate(perm3)) @ matrix(c3)
. Note thatperm2
is applied afterperm3
, because when the second pass ran, it only consideredc2
and notc2 & perm2
, so its "undo" permutationperm3
gets applied beforeperm2
. Clearly, then the complete permutation that we want to store in thePropertySet
is just the effect of applyingperm3
followed by the effect of applyingperm2
- a straightforward permutation composition. Every pass that produces a permutation is responsible for updating the final permutation with that composition (we can supply a helper method that's justPermutation.compose
as I had it above, plus handling for if the second permutation isNone
).dag.qubits
in any way (i.e. any layout pass, or any ancilla-expansion pass), those passes are clearly responsible for updatingfinal_permutation
so that the invariant of "dag.apply_operation_back(PermutationGate(property_set["final_permutation"]), dag.qubits))
is how to undo the mid-circuit permutation" remains correct - i.e. they're responsible for updating the stored permutation so it stays in sync withdag.qubits
, since they're changingdag.qubits
.property_set["final_permutation"]
, when it comes time to save the permutation at the end, we don't actually have to do any work. It doesn't matter whether any layout pass ran, or whether one or more virtual-qubit swap elision passes ran, or any physical-qubit swap mapping passes (etc etc etc) ran - we never need to worry about what the permutation refers to, because it's always whatever happens to be indag.qubits
and it's always (implicitly) applied asPermutationGate(perm)
on all ofdag.qubits
to undo the mapping.In that form, it really doesn't matter if
ApplyLayout
actually runs or not. If it does, it's responsible for updating the permutation to match indices intodag.qubits
, just like it's responsible for updating every gate on the circuit. If it doesn't, then nothing ever changed the meaning of indices intodag.qubits
, so the permutation still has the correct meaning when we reach the end of the transpiler pipeline. We can then map that to the awkwardfinal_layout
form in order to built aTranspileLayout
- I think that'sLayout(zip(dag.qubits, property_set["final_permutation"]))
(regardless of whether routing ran or not), but I don't 100% remember howfinal_layout
currently works. That should then automatically work withTranspileLayout.routing_permutation
.(Fwiw, if we do do this, then I'd tweak my suggestion in #12057 (comment) to use
TranspileLayout.routing_permutation()
instead, since that might sometimes be set withoutinitial_index_layout()
orfinal_index_layout()
having values.)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 Jake, I am now very much in favor of the changes that you are proposing. If we think of the "final_layout" not as of a layout (i.e. not as of a map$V\to P$ ) and not as of a routing permutation of physical qubits (i.e. not as of a map $P\to P$ ), but as representing a permutation that is implicitly applied at the end of the current circuit, then everything can be realized just you have described and we don't need to explicitly store the "virtual_permutation_layout" or introduce the
FinalizeLayouts
pass. Great! This is actually how I was thinking about what's going on all along (i.e. a circuit with an implicit permutation gate at the end), but then tried to fit this into how I understood the "layout" / "final layout" framework.However, in order to avoid the same confusion in the future, can we actually replace "final layout" from
Layout
class by an actual permutation (both choices as a list of integers e.g.[2, 3, 1, 0]
or as a permutation gatePermutationGate([2, 3, 1, 0])
look fine for me) and not as of an object of typeLayout
? I am afraid that technically this change may not be backward-compatible, however these are somewhat of internal transpiler objects, and in theory the user should only be working with the finalTranspileLayout
. What do you think?On the topic, where in Qiskit do we document what each property set attribute really means? For instance, I don't see anything relevant when searching for
final_layout
.Aside: when talking about permutations we have two different conventions going on. IIRC, a
routing_permutation
inTranspileLayout
of the form[2, 3, 1, 0]
means0 -> 2, 1 -> 3, 2 -> 1, 3 -> 0
while forPermutationGate([2, 3, 1, 0])
the convention is2->0, 2->1, 1->2, 0->3
. The two are inverse of each other, but it's still annoying to recall which one is used when.I completely agree on the details of composing permutation and applying permutations to layouts. However, I want a common place that implements these methods, rather than having many passes duplicating the same code. This is why I was hoping that the passes could work with
TranspileLayout
class which already has (or should have) relevant usability functions.I think we also agree that every pass implicitly takes the triple
(current dag, current layout, current implicit permutation)
and is responsible for producing the triple(new dag, new layout, new implicit permutation)
which should be unitary-equivalent (using theOperator.from_circuit
logic) provided that the circuit only consists of unitary operations and there is no ancilla expansion.Back to the backwards-compatibility question and documentation, we would not be telling the writers of say routing passes (that can be written as external routing plugins) to make sure to update the "layout" and "final_layout" attributes. Is this backward-compatible? Where do or should we document this?
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.
For backwards compatibility, I think something like this could work:
final_permutation
a different property tofinal_layout
with the type we want it to havefinal_layout
, but only setfinal_permutation
TranspileLayout
construction at the end of the circuit, iffinal_layout
is set, then assume that it ran during a user-defined routing stage and compose its implied permutation withfinal_permutation
. We may well get that wrong if we ran a swap-elision pass after a user routing pass, but I think that's very unlikely.layout
(since that's just the initial layout), onlyfinal_permutation
.For different conventions between
routing_permutation
andPermutationGate
: that's part of what I was getting at with "we can either store the inverse or the forwards permutation". You could argue that they're the same permutation, it's just one is stored as an additive (apply this to a circuit to undo the induced permutation) and one is subtractive (this is the induced permutation, so apply the inverse of it to undo it). We can makefinal_permutation
store whichever of those matches the convention ofrouting_permutation
already - as you say, you can easily convert one to the other - so we're consistent with ourselves.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.
What does the
t_
prefix stand for here? For readability, can we write it 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.
I'm not sure what his intent was with the prefix either, but we can always clean it up in a follow up 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.
From your discussion above @jakelishman , it sounds to me like we ought to consider keeping any new properties private until we i.e. refactor to storing
final_permutation
.Thoughts?
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 fine not documenting it explicitly as part of the API although at the end of the day the pass doesn't do much in the absence of a virtual permutation layout. So it's kind of hard to keep it private as part of the pass. I don't think it's adding an extra burden realistically to support it for the remainder of 1.x if we pivot to something else in the future we have to do this already with
final_layout
.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.
Yeah, part of us not being able to get the
final_permutation
landed properly for 1.1 is that basically have to support this for the whole 1.x cycle now, but that was priced into the decision - it shouldn't be too painful, and we'd have needed a 2.0 release to make a truly clean break from external routing passes anyway.