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

adding an option to CouplingMap.reduce to allow disconnected coupling maps #10863

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Sep 19, 2023

Summary

This PR adds an option check_if_connected to CouplingMap.reduce, defaulted to True. This corresponds to the previous behavior, checking whether the reduced coupling map remains connected and raising a CouplingError if not so. When set to False, the check is skipped, allowing disconnected reduced coupling maps. This is useful for synthesis plugins (in particular for #10657) where no connectivity assumptions are necessary (for example, we may want to collect and to resynthesize blocks of consecutive swap gates).

Details and comments

This blocks #10657.

The check whether the reduced coupling map is (weakly) connected is now performed only when the option check_if_connected is set to True.

The actual check now uses the CouplingMap's method is_connected instead of scipy. There was no noticeable change in performance.

One additional minor point is that the reduced coupling map should have the correct number of vertices and either coupling_map.add_physical_qubit or coupling_map.graph.add_node should be used. Previously nodes with no edges would not have been added.

@alexanderivrii alexanderivrii requested a review from a team as a code owner September 19, 2023 16:21
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@alexanderivrii alexanderivrii added this to the 0.45.0 milestone Sep 19, 2023
@mtreinish mtreinish self-assigned this Sep 19, 2023
@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Sep 19, 2023
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.

This LGTM, this is straightforward and a good addition to both not change the documented behavior and enable broader use of the reduce method. This was something we actually discussed in #9710 but rolled it back because it was too big an api change for an already large PR.

I had one inline comment about an improvement to efficiency we can make. But feel free to tag this as auto merge and we can make that change in follow up PR.

@@ -281,17 +279,17 @@ def reduce(self, mapping):
if edge[0] in mapping and edge[1] in mapping:
Copy link
Member

Choose a reason for hiding this comment

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

This is pre-existing but we don't have to fix it here. But this would be more efficient if we did this as a set lookup because this will traverse the mapping list twice for each for each edge. rustworkx has a subgraph()` method on graphs, but it doesn't preserve the order of the nodes it receives. But we can at least get a speedup here if we did something like:

subgraph_nodes = set(mapping)
for edge in self.get_edges():
    if edge[0] in subgraph_nodes and edge[1] in subgraph_nodes:

@alexanderivrii alexanderivrii added this pull request to the merge queue Sep 20, 2023
@alexanderivrii
Copy link
Contributor Author

Thanks! I have tried the performance improvement suggestion along with other strategies, such as creating a bitmask for mapping, allowing O(1) lookup (as per the code below), but did not see any difference on my laptop.

in_mapping = [False] * (len(coupling_map.graph.node_indices()))
for val in mapping:
    in_mapping[val] = True
for edge in coupling_map.get_edges():
    if in_mapping[edge[0]] and in_mapping[edge[1]]:

For large graphs with many edges, it seems that > 95% of the time is spent on iterating over the edges (for edge in coupling_map.get_edges() ...) rather than on checking whether the edge remains in the reduced graph, though I might be wrong about this. I will tag this for auto merge as is, but I do agree that it's worth to investigate possible performance improvements.

Merged via the queue into Qiskit:main with commit 01bb111 Sep 20, 2023
14 checks passed
@alexanderivrii alexanderivrii deleted the coupling-map-reduce branch October 23, 2023 07:26
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.

3 participants