Token swapper permutation synthesis plugin

[alexanderivrii]

Summary

This PR implements a new HighLevelSynthesisPlugin for permutations (objects of type PermutationGate) based on Qiskit's token swapper algorithm.

Details and comments

In #10477 we have adjusted the APIs for the high level synthesis transpiler pass and the associated high level synthesis plugins to work with coupling maps/targets.

In the "abstract synthesis" flow, a high-level-object is synthesized before layout/routing, in which case the synthesized circuit is not supposed to adhere to the connectivity constraints (and indeed we don't know yet where the qubits are going to be mapped). This is how HighLevelSynthesis is currently used in Qiskit's transpiler flow.

In the "concrete synthesis" flow, a high-level-object is synthesized after layout/routing, in which case both the coupling map and the set of qubits over which the object is defined are known, and the synthesized circuit is supposed to adhere to these constraints (note that a plugin can return None when it is not able to synthesize). This is not currently used in Qiskit's transpiler flow, but can be potentially integrated into the optimization loop. For example, we could collect blocks of consecutive SWAP gates, merge these blocks into PermutationGate objects, and resynthesizing these permutation gates.

This is the first HighLevelSynthesisPlugin (not counting the attempt in #9250) that is able to work both before and after layout/routing.

In more detail, this PR exposes the Qiskit's token swapper algorithm as a synthesis plugin for permutations. Note that token swapping code is able to handle general coupling maps.

There is one simplifying assumption in the case of "concrete synthesis" flow: the plugin only works when qubits represents a connected subset of coupling_map (if this is not the case, the plugin outputs None). The reason for this assumption is that if the synthesized circuit involves additional qubits (not present in the set of qubits over which the permutation is defined), replacing the node in the DAGCircuit that defines the permutation by the corresponding definition becomes problematic.

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[mtreinish]

This is a really cool plugin, I had a couple questions inline. Also I think this probably needs a release note too to document the new feature.

[mtreinish]

Should seed ben an input option here? The other similar thought is the parallel_threshold argument to ApproximateTokenSwapper.map might also be worth exposing through the options interface, although that one is less clear to me because it's mostly just a function of runtime performance and not the actual synthesis quality.

[alexanderivrii]

Thanks, I have exposed both seed and parallel_threshold.

[mtreinish]

Is the list cast here necessary? The EdgeList return should be iterable and should work in the for loop at L470 without issue. Doing a list cast here results in double iteration over the output swap list.

[alexanderivrii]

Fixed.

[coveralls]

Pull Request Test Coverage Report for Build 5900518856

• 26 of 26 (100.0%) changed or added relevant lines in 1 file are covered.
• 10 unchanged lines in 2 files lost coverage.
• Overall coverage remained the same at 87.267%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	91.14%
crates/qasm2/src/parse.rs	6	96.67%

Totals
Change from base Build 5893570960:	0.0%
Covered Lines:	74382
Relevant Lines:	85235

---

💛 - Coveralls

[mtreinish]

This LGTM, I had one question inline, it's not really a blocker but more I'm wondering your thoughts about a potential path for handling disconnected coupling maps.

[mtreinish]

We should be able to handle disconnected subgraphs if we work with the graph object directly. Something like:

graph = coupling_map.graph.subgraph(qubits).to_undirected()

The only issue with that is I don't think there is any guarantees on the index ordering from PyDiGraph.subgraph(). So you might need to do:

for i in coupling_map.graph.node_indices():
    coupling_map.graph[i] = i
graph = coupling_map.graph.subgraph(qubits).to_undirected()

then you you can use graph[j] to get the original index i. That being said I don't know what the approximate token swapper does if it can't fulfill the permutation because the graph is disconnected, so this might be the correct behavior anyway.

[alexanderivrii]

Thanks, this is a very interesting suggestion. By experimenting with the approximate token swapper, it seems that if there is a way to fulfill the permutation (on a disconnected subgraph), it will do so; but if there is no such way, it will throw a rather strange error pyo3_runtime.PanicException: IndexMap: key not found.

graph = rx.PyGraph()
graph.extend_from_edge_list([(0, 1), (2, 3)])
swapper = ApproximateTokenSwapper(graph)
swapper.map({1: 0, 0: 1, 2: 3, 3: 2}, 10)  # this works fine
swapper.map({2: 0, 1: 1, 0: 2, 3: 3}, 10)  # this throws 

Question: wrapping the second in the try-except block, outputs the following message (in pycharm):

thread '<unnamed>' panicked at 'IndexMap: key not found', C:\Users\274191756\Desktop\QiskitDevelopment\rustworkx\rustworkx-core\src\token_swapper.rs:211:16
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Would this be a problem?

Another question: is it safe to modify the original coupling map: coupling_map.graph[i] = i, or would it be best to make a copy of that first?

[mtreinish]

Hah, well that's a bug in rustworkx. A PanicException is the catch all for a Rust panic or an unhandled error and we shouldn't be panicking with an otherwise valid input. Catching a PanicException isn't super straightforward because of design decision in PyO3 it inherits from BaseException (which is the parent to Exception) to make it on the same level as other low level exceptions that aren't generally recoverable. The thinking being a panic is explicitly an unhandled error so nothing in Python should be able to deal with it. So I wouldn't try to catch it, we'll have to fix it in rustworkx.

As for coupling_map.graph[i] = i we don't explicitly reserve the use of the node weights for anything and if something does depend on them (like CouplingMap.connected_components()) will overwrite it. Although doing a graph copy is probably fairly lightweight as it's just a rust clone internally. So something like coupling_map.graph.copy() is probably a good idea.

[mtreinish]

I pushed up a fix for this in rustworkx: Qiskit/rustworkx#971

[alexanderivrii]

I really want to support disjoint coupling maps in this and other synthesis plugins.

First, as explained in #10657 (comment), using coupling_map.graph.subgraph(qubits) instead of coupling_map.reduce(qubits) does not preserve the index ordering. This leads to a somewhat cumbersome solution that involves remapping the desired permutation to be over the subgraph's qubits (this is the input to the token swapper) and remapping the computed swaps (the output of token swapper) to be over the original qubits. This can be done, but the code is a bit tricky. Furthermore, every other synthesis plugin that is able to support disjoint coupling maps would also need to implement a similar tricky solution.

Do I understand correctly that disconnected coupling maps are now allowed in Qiskit? So an alternative would be to add an argument (something like allow_disconnected) to CouplingMap.reduce that would not error on disconnected coupling maps.

There are a few more minor changes required for CouplingMap.reduce. As an example, let's say that the original map is a ring over 8 qubits, and we want to reduce to qubits=(1, 3), the important thing is that some of the qubits become edgeless in the reduced map. The current code would not add such edgeless qubits to the reduced coupling map (as it adds new qubits when adding edges), but we should in fact add these.

@mtreinish, if you agree with the CouplingMap.reduce suggestions above, would it make sense to add these as a part of this PR or a separate PR?

The second problem is that there is still one edge case when TokenSwapper panics when handling disconnected coupling maps (in addition to Qiskit/rustworkx#971):

def test_disjoint_graph(self):
    graph = rx.PyGraph()
    graph.add_node(0)
    graph.add_node(1)
    graph.add_node(2)
    graph.add_node(3)
    swaps = rx.graph_token_swapper(graph, {1: 0, 0: 1, 2: 3, 3: 2}, 10, seed=42)

This panics as per

thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0', C:\Users\274191756\Desktop\QiskitDevelopment\rustworkx\rustworkx-core\src\token_swapper.rs:304:40

I would be happy to look into this. One additional question is whether we need to wait till the new version of rustworkx is released (including PR 971 and possibly additional fixes) before this PR could be merged?

[coveralls]

Pull Request Test Coverage Report for Build 7715494107

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

• 0 of 26 (100.0%) changed or added relevant lines in 1 file are covered.
• 4 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.05%) to 89.498%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	91.94%

Totals
Change from base Build 7709087344:	0.05%
Covered Lines:	59668
Relevant Lines:	66670

---

💛 - Coveralls

[alexanderivrii]

This probably fixes a minor bug in the existing code, though previously after defining an empty circuit qc = Quantum(3), the check if qc: would return True, but now it returns False.

[mtreinish]

Overall thgis LGTM, thanks for sticking with this while we sorted out the issues in rustworkx. I left one small inline comment about the docs; the only other open question for me is do you think we should raise the minimum rustworkx version because of this. The plugin will work with rustworkx 0.13.x, but in the case of a disconnected graph where there is no solution it will raise an unhandled panic exception. That seems like enough of an edge case to not necessarily warrant requiring a version bump, but it might just be prudent to avoid potential issues. I'm fine either way and will defer to your opinion. If you do want to raise the requirement to rustworkx>=0.14.0 just also throw in an upgrade note to your release note file that documents the change.

[mtreinish]

We need to add this class somewhere in the toctree to render the docstring here. Right now this class (nor any of the HLS plugins) is not being included in the documentation builds. I was talking to @Cryoris offline about this a bit the other day as there isn't a unified place to document this right now things are spread out a bit too much in the organizational structure. I think for right now if you added a HLS Plugins section to the module docstring in qiskit/transpiler/passes/synthesis/plugin.py and added an autosummary for this class that'd be enough. We can refactor the organizational structure in a follow up easily enough.

[alexanderivrii]

@mtreinish, what is the right way to fix import errors in the autosummary:
https://dev.azure.com/qiskit-ci/qiskit/_build/results?buildId=53976&view=logs&j=a15d9aca-d75c-55b1-7f76-889cfe884cf0&t=154f6391-7dd7-5733-2230-2f10f0798b76&l=198

[mtreinish]

The root issue is that for autosummary to work the path has to be in the current context. Sphinx is basically trying to do the equivalent of __module__.ACGSynthesisPermutation from the module of the autosummary directive to get the docstring for the class, but ACGSynthesisPermutation hasn't been imported and it errors because it can't find that attribute. You can use a more explicit path to include it in the docs from there or alternatively import the class. I'll play with it locally and see if I can come up with a quick workaround, as I'd rather not block this PR on including the plugin class in the docs as no other synthesis plugins except for aqc are.

[mtreinish]

LGTM (assuming I didn't make any typos in my small docs tweak, the local build env for me got corrupted and I didn't want to mess with it), thanks for the quick updates.

[alexanderivrii]

Thanks Matthew!