Use retworkx for CouplingMap #5183
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit migrates Terra's CouplingMap class to us retworkx internally instead of networkx providing >10x speed improvement for CouplingMap operations. Requires: Qiskit/rustworkx#157 Qiskit/rustworkx#156 Qiskit/rustworkx#144 Qiskit/rustworkx#143 Qiskit/rustworkx#147 Qiskit/rustworkx#158 Qiskit/rustworkx#162 Qiskit/rustworkx#161 all be applied to the retworkx version installed.
|
Ok, retworkx 0.6.0 was released this morning, so this should be good to merge now. |
Cryoris
reviewed
Dec 2, 2020
Comment on lines
+113
to
+114
| graph.add_edges_from_no_data( | ||
| [(i, i + 1) for i in range(len(graph) - 1)]) |
There was a problem hiding this comment.
What's the difference to extend_from_edge_list?
There was a problem hiding this comment.
extend_from_edge_list() will add nodes to the graph if there is a node index in the edge tuple that doesn't exist. While add_edges_from_no_data() will error if there is an index in the edge tuple that doesn't exist.
Co-authored-by: Julien Gacon <gaconju@gmail.com>
|
retworkx 0.7.0 was released this morning (and 0.7.1 a few minutes ago to fix an issue this PR uncovered with it around pickling) and added generator functions that can be used for |
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Dec 4, 2020
In Qiskit#5183 we switched the internal graph datastructures used for the CouplingMap to be based on retworkx instead of networkx. That PR was written against the 0.6.0 retworkx release and for the from_grid() and from_full() constructors python had to be used since retworkx was missing functions to generate graphs of those types. However, in the recent 0.7.x retworkx release 2 new generators directed_grid_graph() [1] and directed_mesh_graph() [2] were added. This commit switches the implementation of from_grid() and from_full() to use these generators, which significantly speeds up (and reduces the memory overhead) of the constructor methods. The only constructor which still relies on a Python loop is the from_full() method when bidirection=False. This is because retworkx doesn't offer a constructor that will build a graph like that. If in a future release a generator is added we can remove that for loop too. [1] https://retworkx.readthedocs.io/en/stable/stubs/retworkx.generators.directed_grid_graph.html [2] https://retworkx.readthedocs.io/en/stable/stubs/retworkx.generators.directed_mesh_graph.html
mergify bot
added a commit
that referenced
this pull request
Dec 4, 2020
In #5183 we switched the internal graph datastructures used for the CouplingMap to be based on retworkx instead of networkx. That PR was written against the 0.6.0 retworkx release and for the from_grid() and from_full() constructors python had to be used since retworkx was missing functions to generate graphs of those types. However, in the recent 0.7.x retworkx release 2 new generators directed_grid_graph() [1] and directed_mesh_graph() [2] were added. This commit switches the implementation of from_grid() and from_full() to use these generators, which significantly speeds up (and reduces the memory overhead) of the constructor methods. The only constructor which still relies on a Python loop is the from_full() method when bidirection=False. This is because retworkx doesn't offer a constructor that will build a graph like that. If in a future release a generator is added we can remove that for loop too. [1] https://retworkx.readthedocs.io/en/stable/stubs/retworkx.generators.directed_grid_graph.html [2] https://retworkx.readthedocs.io/en/stable/stubs/retworkx.generators.directed_mesh_graph.html Co-authored-by: Luciano Bello <luciano.bello@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Dec 7, 2020
The topological_code fitters module relies on networkx and has had a hard dependency on it since it was first introduced in qiskit-community#211. However, it was never added to the requirements list. This was never caught because historically qiskit-terra (which is in the requirements list) has required networkx too so installing qiskit-terra would install networkx. But, in Qiskit/qiskit#5183 the dependency on networkx was removed from terra. This commit corrects the issue so that we're properly listing networkx as an ignis requirement moving forward. Longer term we should migrate the topological codes fitter to use retworkx for better performance and consistency with the rest of Qiskit. However, before we can do that Qiskit/rustworkx#216 must be fixed first.
mtreinish
added a commit
to qiskit-community/qiskit-ignis
that referenced
this pull request
Dec 7, 2020
The topological_code fitters module relies on networkx and has had a hard dependency on it since it was first introduced in #211. However, it was never added to the requirements list. This was never caught because historically qiskit-terra (which is in the requirements list) has required networkx too so installing qiskit-terra would install networkx. But, in Qiskit/qiskit#5183 the dependency on networkx was removed from terra. This commit corrects the issue so that we're properly listing networkx as an ignis requirement moving forward. Longer term we should migrate the topological codes fitter to use retworkx for better performance and consistency with the rest of Qiskit. However, before we can do that Qiskit/rustworkx#216 must be fixed first.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Dec 11, 2020
In Qiskit#5183 the inner graph object of the CouplingMap object was switched to retworkx. While generally faster there are a few different edge cases where retworkx performance can be worse than networkx. Namely, any place where large lists are returned, for example lists of all nodes or edges in a graph. In those cases the conversion from the Rust objects to the Python objects can culmulatively be expensive. To avoid this conversion overhead in every case retworkx has custom return types that are python sequences but defer type conversion to __getitem__, meaning that the return is fast but complete traversal can be slow, epsecially when done multiple times. In such cases it's easier to cast the sequence to a Python object such as a set or a list so the conversion is only done once. However, in the CSPLayout pass the edges list returned from retworkx was searched multiple times as part of the constraint function which resulted in a large performance regression. This commit fixes this by casting the local edges to a set(). This has 2 advantages, first it avoids the multiple traversals of the graph which removes the conversion overhead. The second advantage is that by using a set the lookups are constant time instead of iterating over the full list every time. This results in a performance improvement over the performance prior to Qiskit#5183 and the introduction of the regression.
kdk
pushed a commit
that referenced
this pull request
Dec 11, 2020
In #5183 the inner graph object of the CouplingMap object was switched to retworkx. While generally faster there are a few different edge cases where retworkx performance can be worse than networkx. Namely, any place where large lists are returned, for example lists of all nodes or edges in a graph. In those cases the conversion from the Rust objects to the Python objects can culmulatively be expensive. To avoid this conversion overhead in every case retworkx has custom return types that are python sequences but defer type conversion to __getitem__, meaning that the return is fast but complete traversal can be slow, epsecially when done multiple times. In such cases it's easier to cast the sequence to a Python object such as a set or a list so the conversion is only done once. However, in the CSPLayout pass the edges list returned from retworkx was searched multiple times as part of the constraint function which resulted in a large performance regression. This commit fixes this by casting the local edges to a set(). This has 2 advantages, first it avoids the multiple traversals of the graph which removes the conversion overhead. The second advantage is that by using a set the lookups are constant time instead of iterating over the full list every time. This results in a performance improvement over the performance prior to #5183 and the introduction of the regression.
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Feb 5, 2021
The topological_code fitters module relies on networkx and has had a hard dependency on it since it was first introduced in qiskit-community#211. However, it was never added to the requirements list. This was never caught because historically qiskit-terra (which is in the requirements list) has required networkx too so installing qiskit-terra would install networkx. But, in Qiskit/qiskit#5183 the dependency on networkx was removed from terra. This commit corrects the issue so that we're properly listing networkx as an ignis requirement moving forward. Longer term we should migrate the topological codes fitter to use retworkx for better performance and consistency with the rest of Qiskit. However, before we can do that Qiskit/rustworkx#216 must be fixed first. (cherry picked from commit 2775689)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This commit migrates Terra's CouplingMap class to us retworkx internally
instead of networkx providing >10x speed improvement for CouplingMap
operations.
Details and comments
Requires:
Use contiguous C arrays instead of fortran layout rustworkx#157Add is_weakly_connected and weakly_connected_components rustworkx#156Add subgraph method to PyGraph and PyDiGraph rustworkx#144Add option to digraph_floyd_warshall to treat input as undirected rustworkx#143Add neighbors methods to graph classes rustworkx#147Make weight_fn optional in adjacency matrix and fw numpy rustworkx#158Add Dijkstra shortest path functions rustworkx#162Add to_undirected method for PyDiGraph rustworkx#161Add distance matrix functions rustworkx#166are merged and released before we can merge this.