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 from_heavy_hex() and from_heavy_square() generator methods to CouplingMap #6959

Merged
merged 18 commits into from
Sep 21, 2021

Conversation

paniash
Copy link
Contributor

@paniash paniash commented Aug 28, 2021

Summary

Fixes #6947.

Details and comments

Leverages newly written retworkx functionsdirected_heavy_hex_graph() and directed_heavy_square_graph() in CouplingMap.

@paniash paniash requested a review from a team as a code owner August 28, 2021 12:13
@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Aug 28, 2021
Copy link
Member

@mtreinish mtreinish left a 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 Show resolved Hide resolved
@@ -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."""
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 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.

qiskit/transpiler/coupling.py Outdated Show resolved Hide resolved
@mtreinish
Copy link
Member

@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?

@paniash
Copy link
Contributor Author

paniash commented Sep 16, 2021

@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. 😃

@mtreinish
Copy link
Member

@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. smiley

I took care of adding a release note and docstring in: 124894e and 37c6a63

qiskit/transpiler/coupling.py Outdated Show resolved Hide resolved
qiskit/transpiler/coupling.py Outdated Show resolved Hide resolved
Co-authored-by: Jake Lishman <jake@binhbar.com>
@jakelishman
Copy link
Member

This is new features only, so is stable for backport if we want it to go in.

@mtreinish
Copy link
Member

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.

@jakelishman
Copy link
Member

Oh, my bad on both points! I'd forgotten about the retworkx bump entirely, actually.

Comment on lines +337 to +339
bidirectional (bool): Whether the edges in the output coupling
graph are bidirectional or not. By default this is set to
``True``
Copy link
Member

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.

image

Copy link
Member

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)

Comment on lines +332 to +336
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.
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering how one might make a coupling map like ibmq_mumbai using this, since it is a 27-qubit graph. Maybe if there was a way to remove some qubits from a coupling map, a user could start with a base and manually work towards that.
image

Copy link
Member

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()

Figure_1

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

Copy link
Member

Choose a reason for hiding this comment

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

With bidirectional=False:

undirected

Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit 123a0a7 into Qiskit:main Sep 21, 2021
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add from_heavy_hex() and from_heavy_square() generator methods to CouplingMap
5 participants