General routing

[eddieschoute]

Summary

From #2860 the question arose if I could submit a PR for the Token Swapping algorithm separately and this pull request does that.
The algorithm allows for a partial permutation to be implemented through a Swap circuit that attempts to minimize circuit size.

Details and comments

[ajavadia]

There is a qiskit.transpiler.passes.routing directory now so it would be better if this was there rather than in the transpiler namespace.

[eddieschoute]

@ajavadia Do you want to algorithm (currently in transpiler.routing.general.py) also in the passes directory? I put the Pass in the top-level of the pass directory as you had suggested since routing seems to have a different definition within the Qiskit project, i.e., it means to map and route a whole circuit.

I implemented the code as a pass as well but I haven't tested it yet.

[ajavadia]

what about creating a directory for transpiler.algorithms? It just seems transpiler.routing and transpiler.passes.routing might lead to confusion. @kdk @1ucian0 thoughts?

[1ucian0]

what about creating a directory for transpiler.algorithms? It just seems transpiler.routing and transpiler.passes.routing might lead to confusion.

For me it is clear that everything in transpiler.passes is a pass and can be used in a pass manager and everything out cannot. So, in principle, I don't see the problem.

However, if this pass is the only one using these algorithms, why not having them together?

[eddieschoute]

I think the issue is that the functionality is useful outside of a pass as well. Therefore it may be odd to put it inside a pass only. I leave it up to you where to put it :)

[1ucian0]

It is useful for transpiler or in general?

[ajavadia]

It is useful for transpiler or in general?

It's useful in multiple transpiler passes.

I leave it up to you where to put it :)

It doesn't matter that much honestly. Just leave it as is.

[ajavadia]

I tried running the pass. I think the DAGCircuit.extend_back() method is buggy, which is preventing the pass from working at the moment. Is this what you find too?

It has this line which just seems wrong as it adds an entry with always the same key/value.

edge_map.update([(qbit, qbit) for qbit in qreg if qbit not in edge_map])

[itoko]

Thank you for preparing this PR, @eddieschoute !

I think from_layout/to_layout (or start_layout/goal_layout?) is less confusing than initial_layout/final_layout that they are reserved as layout at the beginning/end of the circuit.
Since LayoutTransformation pass appends swaps at the end of circuit, the starting layout is often the "final_layout". To avoid confusion, I recommend to use the other terms.

I also think "from_layout" and "to_layout" can be taken in run time i.e. within run() so that this pass can cooperate with the other passes. In many cases, the "from_layout" (in this pass, the layout at the end of the circuit) is given after running a routing pass. I think we should be able to get the "from_layout" (or "to_layout") from property_set in run(). (I know this is impossible for now.)

To this make possible, I think all of the routing passes should set property_set["final_layout"]. It would be useful for other passes (I have one concrete example in my mind). Thoughts? @ajavadia , @1ucian0 , @kdk

(+1 to create a directory for transpiler.algorithms)

[eddieschoute]

While you work on #3769, is there an alternative way of adding the Swaps to the end of the DAG?

I think from_layout and to_layout sound good. I would've preferred to put them as arguments in the run() but that wasn't feasible.

[ajavadia]

I tested this on top of PR #3772 and it seems to work (Suggested a couple more bug fixes inline)

[itoko]

While I work on #1803, I've used apply_operation_back to append swap gates (on physical qubits) to the end of the dag.

new_dag.apply_operation_back(SwapGate(), [pqubit_a, pqubit_b])

How about to use it?
It works well for routing passes that return circuits with physical qubits. I think it should work for the LayoutTransformation pass as well. (You've already use it within util.circuit())

Sorry, I confused. Is input/output dagcircuit of the LayoutTransformation pass virtual or physical? Both are virtual? Agnostic?

Excuse me for my unclear message, I meant something like

def __init__(self, coupling_map, from_layout: Union(str, Layout), to_layout):
    self.from_layout = from_layout
    ...

def run(dag):
    from_layout = self.from_layout
    if isinstance(from_layout, str)
        from_layout = self.propertye_set[from_layout]
    ...

(Because it's not allowed to take any arguments other than dag in run().)

[eddieschoute]

I'm not quite sure what you mean but I think the situation that makes most sense is when you have a DagCircuit that somehow respects the physical constraints and you wish to move the virtual qubits to different physical qubits (the to_layout) than what they were before (the from_layout).

[eddieschoute]

I'm running into this linter error

test/randomized/test_transpiler_equivalence.py:1:0: R0401: Cyclic import (qiskit -> qiskit.execute -> qiskit.compiler.transpile -> qiskit.transpiler.transpile_circuit -> qiskit.transpiler.preset_passmanagers -> qiskit.transpiler.preset_passmanagers.level2 -> qiskit.transpiler.passes -> qiskit.transpiler.passes.routing -> qiskit.transpiler.passes.routing.layout_transformation -> qiskit.transpiler.routing.util) (cyclic-import)

but I don't see how that is cyclic. I only import types from the same module (through the init). Any suggestions?

[eddieschoute]

Running into a cyclic import again it seems...

[itoko]

@eddieschoute I succeeded to reproduce cyclic import in my environment and found a workaround, it can be resolved by moving transpiler/routing directory to transpiler/passes/routing/algorithms. I know we need further discussion on the right place for algorithms used in more than routing.
I have several ideas to improve APIs around LayoutTransformation pass.
For example, I'd like to put all the files in the transpiler/routing into one file and name it token_swapper.py at the point of this PR.
If you don't mind, I'd like to apply those tweaks directly to your branch. (If you don't like them, you can revert them.) Are you OK with it?

[eddieschoute]

That's fine with me. The util functions I factored out of my previous PR #2860 but within this PR do not serve a purpose that is shared across multiple files. Just note that it is in #2860 so would likely need to be factored back out for that PR. So I'm not sure it's worth combining them into one file now.

[itoko]

OK. Regarding file changes, I had second thought, to make my tweak minimal, I'll leave the two files types.py and util.py as separated files. I'll apply other tweaks to your branch.

[itoko]

@eddieschoute I've done with my API tweaks. Can you check if they make sense to you?

[eddieschoute]

I think they make sense. One nitpick I have is that we call the file token_swapper. I think for anyone outside the project they would not know what it does without reading the docs. It's why I chose the routing folder structure. Should we choose to rename the class to GeneralRouting or something similar instead?

[itoko]

Good. It's better to make the names more user-friendly. I don't stick the name token_swapper.
GeneralRouting might be too general. I prefer including the term "permutation/permuter" (GraphNodePermuter or something like that), or "partial routing" if you want to use the term "routing". The term routing usually means complete mapping onto physical circuit while the token swapping algorithm does permutation of nodes (= physical qubits) on coupling graph (i.e. partial mapping that changes layout, not always complete mapping of whole circuit).

[ajavadia]

Looks good. Please remove the WIP if it is ready.

[ajavadia]

Ok thanks I think this is good to go