Add support to CouplingMap for disjoint qubits

[mtreinish]

Summary

Previously the CouplingMap class only supported graphs which were fully connected. This prevented us from modeling potential hardware which didn't have a path between all qubits. This isn't an inherent limitation of the underlying graph data structure but was a limitation put on the CouplingMap class because several pieces of the transpiler assume a path always exists between 2 qubits (mainly in layout and routing). This commit removes this limitation and also adds a method to get a subgraph CouplingMap for all the components of the CouplingMap. This enables us to model these devices with a CouplingMap, which is the first step towards supporting these devices in the transpiler.

One limitation with this PR is most fo the layout and routing algorithms do not support disjoint connectivity. The primary exception being TrivialLayout (although the output might be invalid) VF2Layout and VF2PostLayout which inherently support this already. This commit lays the groundwork to fix this limitation in a follow-up PR but for the time being it just raises an error in those passes if a disconnected CouplingMap is being used. The intent here is to follow up to this commit soon for adding support for SabreLayout, SabreSwap, DenseLayout, and StochasticSwap to leverage the method get_component_subgraphs added here to make them usable on such coupling maps.

Details and comments

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

• @Qiskit/terra-core

[nonhermitian]

I worry what these changes mean in the grander scheme of what is means to be a device with num_qubits. In this case CouplingMap.size() gives a single number, but that size no longer corresponds to what we would typically call the number of qubits. The change is completely fine for the case of abstract graphs, but in the context of a quantum computing device it is a bit tricky to deal with

[jakelishman]

I'm not sure what you would instead prefer to call the qubit count? Something like Heron is (to me) explicitly a 399q device which is made of 3x 133q interconnected components, which this PR also adds a method to access as separate maps, so both 399 and (133, 133, 133) are things we have access to. That seems to satisfy both interpretations to me; you can see how big the device is at large, and how big any individual components are.

[mtreinish]

Well we know that we'll need to be able to model these devices at some point and in the context here CouplingMap.size() still accurately reflects that you have n qubits to use on the target backend, but it isn't a complete picture anymore once we support devices that aren't connected graphs, there isn't a way around that and there isn't anything precluding have an n qubit circuit as long as we don't required connectivity between more than CouplingMap.largest_connected_component() qubits in the circuit (it's a bit more nuanced than this in practice I think). If we do that should error in transpile() just like it does today if the circuit width is more than CouplingMap.size().

The way I was planning to handle this in the layout and routing passes is basically just do something like:

component_graphs = cmap.get_component_subgraphs()
if len(component_graphs) > 1:
    im_graph = build_interaction_graph(dag)
    im_graph_components = rx.weakly_connected_components(im_graph)
    if len(im_graph_components) > 1:
        for im_component in im_graph_components:
            subgraph = im_graph.subgraph(im_compoment)
            # figure out how to map circuit components on to subgraphs
            ...
            self.run_pass(subgraph, graph_components[selected_index])
    else:
        if all(component_graph.size() < len(im_graph) for component_graph in component_graphs):
            raise TranspilerError
        #   Maybe heuristically select which component to run on if there is different sizes or connectivity (VF2PostLayout will correct for error rates)
        self.run_pass(dag, component_graphs[0])
else:
    self.run_pass(dag, cmap)

[nonhermitian]

Ok I guess it is semantics really at the end of the day. We are relaxing the criteria that the number of qubits in a device is equal to the width of an entangled circuit that I could run. E.g. as an extreme example, one could have a 1000Q system, but be able to only run 1Q gates.

[jakelishman]

Minor API break aside, I'm fine with this, but I'll leave it untagged for merge until @kdk and others in the core compiler team have had a chance to look.

[coveralls]

Pull Request Test Coverage Report for Build 4492675907

• 24 of 27 (88.89%) changed or added relevant lines in 5 files are covered.
• 2 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.02%) to 85.368%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/layout/csp_layout.py	2	3	66.67%
qiskit/transpiler/passes/layout/sabre_layout.py	1	2	50.0%
qiskit/transpiler/passes/layout/trivial_layout.py	1	2	50.0%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/layout/dense_layout.py	1	96.97%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	94.64%

Totals
Change from base Build 4492553856:	-0.02%
Covered Lines:	67887
Relevant Lines:	79523

---

💛 - Coveralls

[jakelishman]

I'm happy to merge, just waiting for further reviewer from core compiler team.

[kdk]

Looks good so far, a few questions below.

[kdk]

I think I understand the goal here, but I'm not sure this is the most intuitive way to give users a mapping from the indices in the returned subgraphs to their indicies in the original CouplingMap. That is, the attribute data in CouplingMap.graph nodes is usually None, and with this change would mostly remain None except in the cases where .components() is called.

I would think this would be frequently needed (e.g. to get qubit properties from the Target on any qubits in a partition, and when re-combining subgraphs back into a whole-device circuit). Would returning a mapping dict from new to old indicies (like is done in DAGCircuit.substitute_node_with_dag https://github.com/Qiskit/qiskit-terra/blob/255337662847f9f22982172c186ff98426a6b626/qiskit/dagcircuit/dagcircuit.py#L1181 ) be easier do use?

[kdk]

Alternately, it seems like we're going through a lot of effort (and users will have to have some extra handling code) to workaround the issue that CouplingMap has to be indexed at zero, and can't have holes. Is now the right time to revisit that (maybe by always using the node attribute data to store the physical qubit index)? This would require a different breaking change to users of CouplingMap.graph, but might be easier to manage in the long term.

[mtreinish]

Actually the data payload for nodes in CouplingMap.graph is not always None it depends on how the CoupingMap was created. If you use the add_physical_qubit() api (also implicitly via add_edge()) it sets the node's payload to the index of the qubit. I thought about using a mapping dict, but it's not as straightforward as what we do in DAGCircuit because we have multiple graphs to work with. It'd basically have to be something like:

{
    0: (0, 0),
    1: (1, 0),
    2: (0, 1),
}

or a list of mappings:

[
    {
        0: 0,
        2: 1,
    },
    {
       1: 0,
    },
]

to indicate which subgraph index this node moved to and what it's corresponding index is in that subgraph. When thinking about how this mapping would be used I thought it would just be easier to have the node tell us what it was originally than having to do the double lookup.

What I'm actually protecting against here is not that everything has to be zero indexed and contiguous in the CouplingMap, but it's that we can't easily preserve indices across new graph object creation. If we wanted to do that I'd need a lot of logic to basically reconstruct the holes exactly to reuse the indices across all the subgraphs which is complex and error prone (but possible) and also a lot slower. It was much easier to just do the mapping once in the outer scope and let rustworkx just give us whatever indices make sense.

[kdk]

Actually the data payload for nodes in CouplingMap.graph is not always None it depends on how the CoupingMap was created. If you use the add_physical_qubit() api (also implicitly via add_edge()) it sets the node's payload to the index of the qubit.

Maybe this is a bug in and of itself. I tried CouplingMap.from_* and CouplingMap(edge_list) neither of which generated indices on the graph nodes.

Agree that having the data on the nodes is preferable to a mapping dict, but I think really only if can be applied consistently. Otherwise, we risk ending up in a state where node data can be any of {always None, the physical qubit index on an originating CouplingMap, the physical qubit index on this CouplingMap} depending on how the CouplingMap was created.

As an alternative, would a move from having the node index to the node data be the source of truth for the physical qubit index be an option? Looking through CouplingMap quickly, I don't immediately see anything that would make this impossible (though I'm sure I'm missing pieces, and there are performance concerns that would have to be evaluated). This wouldn't I think be a change in existing behavior since for connected CouplingMaps, these would always be the same.

If that's not possible, I think returning a list of dicts that is already zipped with or is zip-able with the list of component graphs would also seem reasonable.

[mtreinish]

I actually did a bit of a check and another place where it's not None currently is in the output from Target.build_coupling_map() (https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/target.py#L839-L863 ). I can't remember why I did that, I assume I was thinking having the gates and error properties on the CouplingMap's graph might be useful, especially for calculating shortest paths over error rates efficiently, so you could do something like rx.dijkstra_shortest_paths(cmap.graph, 0, 2, weight_fn=lambda props: sum(x.error for x in props.values())). But nothing is really using it at least internally. So, I think we can safely say the node (and edge) payloads use is undefined at this point since we have 3 different use cases in tree depending on how it's created.

At the high level (which is wider scope than this PR) I think there are 2 ways to move forward here we can say the contents of these payloads on CouplingMap.graph continued to be undefined and their use is application specific and just document that explicitly at the class level. The other is to explicitly say that the node and edge payload is the canonical index for the node or edge on the backend. In general this will always be the rustworkx graph's index (e.g cmap.graph[3] == 3), the exception being those generated from this method which is doing this mapping. I'd really like to avoid as general practice having graph index != qubit index as I expect that will lead to more bugs unless you're explicitly handling it.

In general we'll want to use the index on the graph instead of the canonical index, we should only be relying on the node payload for mapping purposes like what is done here. The reason is if we're using the inner graph with any rustworkx functions (including distance_matrix()) the indices used are based on the graph index. I view this use as more of an edge case than something that will be used more generally outside of this. I'm not super worried about someone accidentally using the CouplingMaps output from this method and accidentally using index 0 as qubit 0 where it's really like qubit 2 globally because we're pretty explicit here in the docstring. The use case I had in mind for this is wrapping the run() method for layout and routing passes to basically split the circuit and coupling maps before and then run the pass for each subgraph and remap the output from the subgraph index to the global index.

That all being said if you would prefer this to be zippable list returns I can work with that, it's just a little extra mapping in the next PR to do it in with a dict instead of via the mapping interface the graph already gives us. The performance will likely be indistinguishable.

[kdk]

That all being said if you would prefer this to be zippable list returns I can work with that, it's just a little extra mapping in the next PR to do it in with a dict instead of via the mapping interface the graph already gives us. The performance will likely be indistinguishable.

I'm good to go ahead with this. I think the discussion around what should live on the nodes of coupling_map.graph and how we handle non-zero-indexed CouplingMaps and holes is an important one, but let's discuss that in a separate issue.

[mtreinish]

Is this a fix for #2314 ?

It will in the sense that you can have separate disjoint sections in a coupling map now. But the CouplingMap object still has the same constraints that all the qubit indices are contiguous starting at 0. That constraint hasn't changed (and it realistically isn't something we could given how the CouplingMap gets used elsewhere).

[mtreinish]

I pushed up #9802 based on top of this PR branch to add the second half of using SabreLayout with a disjoint coupling map, if people are interested in seeing how this is used in practice inside the transpiler.

[kdk]

LGTM, thanks for the updates.

[nkanazawa1989]

Why did you keep this validation? @mtreinish
https://github.com/Qiskit/qiskit-terra/blob/a1a67a11741000f8f3b6d6a8766dab7e3cd06847/qiskit/transpiler/coupling.py#L308-L309

[nkanazawa1989]

Ah okey I found it's written in a3cf7ff. It would be great if you could add some option to disable this check, e.g. in QE we (may) want to have reduced coupling map for experiment qubits, which might be disconnected.

[mtreinish]

Ah okey I found it's written in a3cf7ff. It would be great if you could add some option to disable this check, e.g. in QE we (may) want to have reduced coupling map for experiment qubits, which might be disconnected.

Sorry I missed this comment at the time. In the short term we can work around this limitation with reduce by calling the deprecated CouplingMap.subgraph() which doesn't have this check. Alternatively you can just do:

subgraph = CouplingMap()
subgraph.graph = cmap.graph.subgraph(nodes)

to do what subgraph does internally. The only practical difference with doing it either of these ways is that the subgraph method (either on CouplingMap or PyDiGraph) doesn't preserve the input node order. I think for 0.25.0 we can look at removing the restriction on the reduce() method.