Skip to content
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 support to CouplingMap for disjoint qubits #9710

Merged
merged 28 commits into from
Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
4d134bb
Add support to CouplingMap for disjoint qubits
mtreinish Mar 2, 2023
b506d88
Remove coupling map connected check from NoiseAdaptiveLayout
mtreinish Mar 2, 2023
6b56517
Change DenseLayout guard to only prevent running when it won't work
mtreinish Mar 2, 2023
25d3379
Rename get_component_subgraphs to components and cache result
mtreinish Mar 2, 2023
4113586
Merge branch 'main' into disjoint-coupling-map
mtreinish Mar 2, 2023
4d293ba
Drop caching of connected components
mtreinish Mar 2, 2023
033b959
Fix check for dense layout to do a valid comparison
mtreinish Mar 2, 2023
3d47405
Merge branch 'main' into disjoint-coupling-map
mtreinish Mar 2, 2023
e50a4c6
Ensure self loops in CouplingMap.distance() return 0
mtreinish Mar 3, 2023
15500e9
Merge remote-tracking branch 'origin/main' into disjoint-coupling-map
mtreinish Mar 3, 2023
9111a7a
Fix lint
mtreinish Mar 3, 2023
850e581
Update CouplingMap.components() docstring
mtreinish Mar 7, 2023
c4f2da5
Merge branch 'main' into disjoint-coupling-map
mtreinish Mar 7, 2023
df6b891
Expand test coverage
mtreinish Mar 7, 2023
8079699
Remove unused option for strongly connected components
mtreinish Mar 7, 2023
654a965
Expand docstring to explain return list order
mtreinish Mar 7, 2023
b847ebc
Use infinity for disconnected nodes in distance matrix
mtreinish Mar 7, 2023
42474df
Update releasenotes/notes/add-cmap-componets-7ed56cdf294150f1.yaml
mtreinish Mar 7, 2023
0936fb0
Rename CouplingMap.components to connected_components()
mtreinish Mar 8, 2023
91b8a0d
Fix typo in relaese note
mtreinish Mar 8, 2023
a4d2133
Merge remote-tracking branch 'origin/main' into disjoint-coupling-map
mtreinish Mar 8, 2023
8957997
Update method name in release note
mtreinish Mar 8, 2023
6ee0c34
Merge remote-tracking branch 'origin/main' into disjoint-coupling-map
mtreinish Mar 15, 2023
55df39e
Merge remote-tracking branch 'origin/main' into disjoint-coupling-map
mtreinish Mar 22, 2023
a3cf7ff
Restore previous reduce() behavior
mtreinish Mar 22, 2023
2d13b6a
Merge branch 'main' into disjoint-coupling-map
mtreinish Mar 22, 2023
acebb43
Add missing import
mtreinish Mar 22, 2023
f7112dd
Merge branch 'main' into disjoint-coupling-map
mtreinish Mar 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 68 additions & 18 deletions qiskit/transpiler/coupling.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
onto a device with this coupling.
"""

from typing import List
import warnings

import numpy as np
import rustworkx as rx
from rustworkx.visualization import graphviz_draw

Expand All @@ -37,7 +37,16 @@ class CouplingMap:
and target qubits, respectively.
"""

__slots__ = ("description", "graph", "_dist_matrix", "_qubit_list", "_size", "_is_symmetric")
__slots__ = (
"description",
"graph",
"_dist_matrix",
"_qubit_list",
"_size",
"_is_symmetric",
"_strong_components",
"_weak_components",
)

def __init__(self, couplinglist=None, description=None):
"""
Expand All @@ -60,6 +69,8 @@ def __init__(self, couplinglist=None, description=None):
# number of qubits in the graph
self._size = None
self._is_symmetric = None
self._strong_components = None
self._weak_components = None

if couplinglist is not None:
self.graph.extend_from_edge_list([tuple(x) for x in couplinglist])
Expand Down Expand Up @@ -100,6 +111,8 @@ def add_physical_qubit(self, physical_qubit):
self._dist_matrix = None # invalidate
self._qubit_list = None # invalidate
self._size = None # invalidate
self._weak_components = None # invalidate
self._strong_components = None # invalidate

def add_edge(self, src, dst):
"""
Expand All @@ -115,6 +128,8 @@ def add_edge(self, src, dst):
self.graph.add_edge(src, dst, None)
self._dist_matrix = None # invalidate
self._is_symmetric = None # invalidate
self._weak_components = None # invalidate
self._strong_components = None # invalidate

def subgraph(self, nodelist):
"""Return a CouplingMap object for a subgraph of self.
Expand Down Expand Up @@ -175,8 +190,6 @@ def compute_distance_matrix(self):
those or want to pre-generate it.
"""
if self._dist_matrix is None:
if not self.is_connected():
raise CouplingError("coupling graph not connected")
self._dist_matrix = rx.digraph_distance_matrix(self.graph, as_undirected=True)

def distance(self, physical_qubit1, physical_qubit2):
Expand All @@ -197,7 +210,10 @@ def distance(self, physical_qubit1, physical_qubit2):
if physical_qubit2 >= self.size():
raise CouplingError("%s not in coupling graph" % physical_qubit2)
self.compute_distance_matrix()
return int(self._dist_matrix[physical_qubit1, physical_qubit2])
res = int(self._dist_matrix[physical_qubit1, physical_qubit2])
if res == 0:
raise CouplingError(f"No path from {physical_qubit1} to {physical_qubit2}")
return res
mtreinish marked this conversation as resolved.
Show resolved Hide resolved

def shortest_undirected_path(self, physical_qubit1, physical_qubit2):
"""Returns the shortest undirected path between physical_qubit1 and physical_qubit2.
Expand Down Expand Up @@ -268,9 +284,6 @@ def reduce(self, mapping):
Raises:
CouplingError: Reduced coupling map must be connected.
"""
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
from scipy.sparse import coo_matrix, csgraph

reduced_qubits = len(mapping)
inv_map = [None] * (max(mapping) + 1)
for idx, val in enumerate(mapping):
inv_map[val] = idx
Expand All @@ -281,16 +294,6 @@ def reduce(self, mapping):
if edge[0] in mapping and edge[1] in mapping:
reduced_cmap.append([inv_map[edge[0]], inv_map[edge[1]]])

# Verify coupling_map is connected
rows = np.array([edge[0] for edge in reduced_cmap], dtype=int)
cols = np.array([edge[1] for edge in reduced_cmap], dtype=int)
data = np.ones_like(rows)

mat = coo_matrix((data, (rows, cols)), shape=(reduced_qubits, reduced_qubits)).tocsr()

if csgraph.connected_components(mat)[0] != 1:
raise CouplingError("coupling_map must be connected.")

return CouplingMap(reduced_cmap)

@classmethod
Expand Down Expand Up @@ -403,6 +406,53 @@ def largest_connected_component(self):
"""Return a set of qubits in the largest connected component."""
return max(rx.weakly_connected_components(self.graph), key=len)

def components(self, respect_direction: bool = False) -> List["CouplingMap"]:
"""Separate a CouplingMap into subgraph Coupling Maps for each connected component.
kdk marked this conversation as resolved.
Show resolved Hide resolved

This method will return a list of :class:`~.CouplingMap` objects for each connected
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
component in this :class:`~.CouplingMap`. The data payload of each node in the
:attr:`~.CouplingMap.graph` attribute will contain the qubit number in the original
graph. This will enables mapping the qubit index in a component subgraph to
the original qubit in the combined :class:`~.CouplingMap`. For example::

from qiskit.transpiler import CouplingMap

cmap = CouplingMap([[0, 1], [1, 2], [2, 0], [3, 4], [4, 5], [5, 3]])
component_cmaps = cmap.get_component_subgraphs()
print(component_cmaps[1].graph[0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@mtreinish mtreinish Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


will print ``3`` as index ``0`` in the second component is qubit 3 in the original cmap.

Args:
respect_direction: If set to ``True`` the connected components will be strongly connected
mtreinish marked this conversation as resolved.
Show resolved Hide resolved

Returns:
list: A list of :class:`~.CouplingMap` objects for each connected
components.
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
"""
if respect_direction and self._strong_components is not None:
return self._strong_components
if self._weak_components is not None:
return self._weak_components

# Set payload to index
for node in self.graph.node_indices():
self.graph[node] = node
if not respect_direction:
components = rx.weakly_connected_components(self.graph)
else:
components = rx.strongly_connected_components(self.graph)
output_list = []
for component in components:
new_cmap = CouplingMap()
new_cmap.graph = self.graph.subgraph(list(sorted(component)))
output_list.append(new_cmap)
if respect_direction:
self._strong_components = output_list
else:
self._weak_components = output_list
return output_list

def __str__(self):
"""Return a string representation of the coupling graph."""
string = ""
Expand Down
6 changes: 6 additions & 0 deletions qiskit/transpiler/passes/layout/csp_layout.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

from qiskit.transpiler.layout import Layout
from qiskit.transpiler.basepasses import AnalysisPass
from qiskit.transpiler.exceptions import TranspilerError
from qiskit.utils import optionals as _optionals


Expand Down Expand Up @@ -60,6 +61,11 @@ def __init__(

def run(self, dag):
"""run the layout method"""
if not self.coupling_map.is_connected():
raise TranspilerError(
"Coupling Map is disjoint, this pass can't be used with a disconnected coupling "
"map."
kdk marked this conversation as resolved.
Show resolved Hide resolved
)
qubits = dag.qubits
cxs = set()

Expand Down
5 changes: 5 additions & 0 deletions qiskit/transpiler/passes/layout/dense_layout.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ def run(self, dag):
raise TranspilerError(
"A coupling_map or target with constrained qargs is necessary to run the pass."
)
if dag.num_qubits() > self.coupling_map.largest_connected_component():
raise TranspilerError(
"Coupling Map is disjoint, this pass can't be used with a disconnected coupling "
"map for a circuit this wide."
)
num_dag_qubits = len(dag.qubits)
if num_dag_qubits > self.coupling_map.size():
raise TranspilerError("Number of qubits greater than device.")
Expand Down
6 changes: 5 additions & 1 deletion qiskit/transpiler/passes/layout/sabre_layout.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,11 @@ def run(self, dag):
"""
if len(dag.qubits) > self.coupling_map.size():
raise TranspilerError("More virtual qubits exist than physical.")

if not self.coupling_map.is_connected():
raise TranspilerError(
"Coupling Map is disjoint, this pass can't be used with a disconnected coupling "
"map."
)
# Choose a random initial_layout.
if self.routing_pass is not None:
if self.seed is None:
Expand Down
5 changes: 5 additions & 0 deletions qiskit/transpiler/passes/layout/trivial_layout.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ def run(self, dag):
Raises:
TranspilerError: if dag wider than self.coupling_map
"""
if not self.coupling_map.is_connected():
raise TranspilerError(
"Coupling Map is disjoint, this pass can't be used with a disconnected coupling "
"map."
)
if dag.num_qubits() > self.coupling_map.size():
raise TranspilerError("Number of qubits greater than device.")
self.property_set["layout"] = Layout.generate_trivial_layout(
Expand Down
13 changes: 13 additions & 0 deletions releasenotes/notes/add-cmap-componets-7ed56cdf294150f1.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
features:
- |
Added support to the :class:`~.CouplingMap` object to have a disjoint
connectivity. Previously, a :class:`~.CouplingMap` could only be
constructed if the graph was connected. This will enable using
:class:`~.CouplingMap` to represent hardware with disjoint qubits.
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
- |
Added a new method :meth:`.CouplingMap.components` which
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
is used to get a list of :class:`~.CouplingMap` component subgraphs for
a disjoint :class:`~.CouplingMap`. If the :class:`~.CouplingMap` object
is connected this will just return a single :class:`~.CouplingMap`
equivalent to the original.
63 changes: 55 additions & 8 deletions test/python/transpiler/test_coupling.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

import unittest

import numpy as np
import rustworkx as rx

from qiskit.transpiler import CouplingMap
from qiskit.transpiler.exceptions import CouplingError
from qiskit.providers.fake_provider import FakeRueschlikon
Expand Down Expand Up @@ -101,14 +104,6 @@ def test_successful_reduced_map(self):
ans = [(1, 2), (3, 2), (0, 1)]
self.assertEqual(set(out), set(ans))

def test_failed_reduced_map(self):
"""Generate a bad disconnected reduced map"""
fake = FakeRueschlikon()
cmap = fake.configuration().coupling_map
coupling_map = CouplingMap(cmap)
with self.assertRaises(CouplingError):
coupling_map.reduce([12, 11, 10, 3])

def test_symmetric_small_true(self):
coupling_list = [[0, 1], [1, 0]]
coupling = CouplingMap(coupling_list)
Expand Down Expand Up @@ -448,6 +443,58 @@ def test_implements_iter(self):
expected = [(0, 1), (1, 0), (1, 2), (2, 1)]
self.assertEqual(sorted(coupling), expected)

def test_disjoint_coupling_map(self):
cmap = CouplingMap([[0, 1], [1, 0], [2, 3], [3, 2]])
self.assertFalse(cmap.is_connected())
distance_matrix = cmap.distance_matrix
expected = np.array(
[
[0, 1, 0, 0],
[1, 0, 0, 0],
[0, 0, 0, 1],
[0, 0, 1, 0],
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
]
)
np.testing.assert_array_equal(expected, distance_matrix)
mtreinish marked this conversation as resolved.
Show resolved Hide resolved

def test_components_connected_graph(self):
cmap = CouplingMap.from_line(5)
self.assertTrue(cmap.is_connected())
subgraphs = cmap.components()
self.assertEqual(len(subgraphs), 1)
self.assertTrue(rx.is_isomorphic(cmap.graph, subgraphs[0].graph))

def test_components_disconnected_graph(self):
cmap = CouplingMap([[0, 1], [1, 2], [3, 4], [4, 5]])
self.assertFalse(cmap.is_connected())
subgraphs = cmap.components()
self.assertEqual(len(subgraphs), 2)
expected_subgraph = CouplingMap([[0, 1], [1, 2]])
self.assertTrue(rx.is_isomorphic(expected_subgraph.graph, subgraphs[0].graph))
self.assertTrue(rx.is_isomorphic(expected_subgraph.graph, subgraphs[1].graph))

def test_strongly_connected_components_disconnected_graph(self):
cmap = CouplingMap([[0, 1], [1, 2], [3, 4], [4, 5]])
self.assertFalse(cmap.is_connected())
subgraphs = cmap.components(True)
self.assertEqual(len(subgraphs), 6)
expected_subgraph = CouplingMap()
expected_subgraph.add_physical_qubit(0)
for i in range(6):
self.assertTrue(rx.is_isomorphic(expected_subgraph.graph, subgraphs[i].graph))

def test_components_disconnected_graph_cached(self):
cmap = CouplingMap([[0, 1], [1, 2], [3, 4], [4, 5]])
self.assertFalse(cmap.is_connected())
subgraphs = cmap.components()
self.assertEqual(subgraphs, cmap.components())

def test_strongly_connected_components_disconnected_graph_cached(self):
cmap = CouplingMap([[0, 1], [1, 2], [3, 4], [4, 5]])
self.assertFalse(cmap.is_connected())
subgraphs = cmap.components(True)
self.assertEqual(subgraphs, cmap.components(True))


class CouplingVisualizationTest(QiskitVisualizationTestCase):
@unittest.skipUnless(optionals.HAS_GRAPHVIZ, "Graphviz not installed")
Expand Down