-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 from_heavy_hex()
and from_heavy_square()
generator methods to CouplingMap
#6959
Conversation
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.
Thanks for doing this, I left a few inline comments. Can you also add a feature release note for this? (see: https://github.com/Qiskit/qiskit-terra/blob/main/CONTRIBUTING.md#release-notes ) and maybe also add a test covering the bidirectional case too.
qiskit/transpiler/coupling.py
Outdated
@@ -318,6 +318,24 @@ def from_grid(cls, num_rows, num_columns, bidirectional=True): | |||
) | |||
return cmap | |||
|
|||
@classmethod | |||
def from_heavy_hex(cls, num_distance, bidirectional=True): | |||
"""Return a connected and directed heavy hex map.""" |
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 having some documentation on how distance is used here would be good. The other generator methods are all pretty self explanatory (ie give me 1000 qubit ring), but the heavy hex (and heavy square) graph are a bit ore involved. Explaining the relationship between distance and the number of qubits would be good. It'd also be good to have a reference to the paper here so people can look up what the graph is.
@paniash is there any update on the update on the status of the documentation improvements and the release note? I'm keen to merge this soon. If you're busy would you mind if I pushed this over the finish line? |
@mtreinish Sorry about the delay, I've been a little busy. I am happy to add the release notes, although, adding docstrings for the new functions requires a little bit of reading. If it's urgent, please feel free to go ahead. 😃 |
I took care of adding a release note and docstring in: 124894e and 37c6a63 |
Co-authored-by: Jake Lishman <jake@binhbar.com>
This is new features only, so is stable for backport if we want it to go in. |
Normally we don't backport new features only fixes: https://qiskit.org/documentation/contributing_to_qiskit.html#stable-branch-policy (although there have been exceptions to that in the past). But we can't actually backport this because it requires retworkx 0.10.x which would be a change in minimum version on terra 0.18.x. |
Oh, my bad on both points! I'd forgotten about the retworkx bump entirely, actually. |
bidirectional (bool): Whether the edges in the output coupling | ||
graph are bidirectional or not. By default this is set to | ||
``True`` |
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.
If bidirectional=False
, what direction is chosen? I think this should be according to the paper too. See fig 10 in https://arxiv.org/pdf/1907.09528.pdf.
The black dots are control, and other colors are target. The edge direction in the graph should always go from control to target. This is for both heavy-hex and heavy-square.
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.
It does follow the diagram from the paper. This was something that I brought up in review on the retworkx PRs: Qiskit/rustworkx#293 (review) and Qiskit/rustworkx#313 (review)
distance (int): The code distance for the generated heavy hex | ||
graph. The value for distance can be any odd positive integer. | ||
The distance relates to the number of qubits by: | ||
:math:`n = \\frac{5d^2 - 2d - 1}{2}` where :math:`n` is the | ||
number of qubits and :math:`d` is the ``distance`` parameter. |
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.
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.
Yeah, I think the only way right now would be to leverage the reduce()
method after calling the heavy hex generator. Something like:
from retworkx.visualization import mpl_draw
from qiskit.transpiler import CouplingMap
cmap = CouplingMap.from_heavy_hex(5)
mumbai_cmap = cmap.reduce([4, 40, 3, 39, 27, 2, 9, 44, 38, 1, 8, 26, 43, 7, 42, 29, 6, 13, 47, 41, 5, 12, 28, 46, 11, 45, 10])
mpl_draw(mumbai_cmap.graph, with_labels=True).show()
It's not the prettiest interface for that though, mostly because the node order retworkx creates is by type (data, syndrome, and flag qubits are created up front based on the number given the distance: https://github.com/Qiskit/retworkx/blob/0.10.2/src/generators.rs#L1692-L1702
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.
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.
LGTM
Summary
Fixes #6947.
Details and comments
Leverages newly written retworkx functions
directed_heavy_hex_graph()
anddirected_heavy_square_graph()
inCouplingMap
.