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 support to CouplingMap for disjoint qubits #9710
Add support to CouplingMap for disjoint qubits #9710
Changes from all commits
4d134bb
b506d88
6b56517
25d3379
4113586
4d293ba
033b959
3d47405
e50a4c6
15500e9
9111a7a
850e581
c4f2da5
df6b891
8079699
654a965
b847ebc
42474df
0936fb0
91b8a0d
a4d2133
8957997
6ee0c34
55df39e
a3cf7ff
2d13b6a
acebb43
f7112dd
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 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 inCouplingMap.graph
nodes is usuallyNone
, and with this change would mostly remainNone
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 inDAGCircuit.substitute_node_with_dag
https://github.com/Qiskit/qiskit-terra/blob/255337662847f9f22982172c186ff98426a6b626/qiskit/dagcircuit/dagcircuit.py#L1181 ) be easier do use?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.
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 ofCouplingMap.graph
, but might be easier to manage in the long term.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.
Actually the data payload for nodes in
CouplingMap.graph
is not alwaysNone
it depends on how theCoupingMap
was created. If you use theadd_physical_qubit()
api (also implicitly viaadd_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 inDAGCircuit
because we have multiple graphs to work with. It'd basically have to be something like:or a list of mappings:
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.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.
Maybe this is a bug in and of itself. I tried
CouplingMap.from_*
andCouplingMap(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 originatingCouplingMap
, the physical qubit index on thisCouplingMap
} depending on how theCouplingMap
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 connectedCouplingMap
s, 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.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 actually did a bit of a check and another place where it's not
None
currently is in the output fromTarget.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 theCouplingMap
's graph might be useful, especially for calculating shortest paths over error rates efficiently, so you could do something likerx.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.gcmap.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 theCouplingMap
s 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 therun()
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.
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 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-indexedCouplingMap
s and holes is an important one, but let's discuss that in a separate issue.