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

Migrate ApproximateTokenSwapper to only use retworkx and make networkx optional #5471

Merged
merged 9 commits into from
Dec 7, 2020

Conversation

mtreinish
Copy link
Member

Summary

This commit removes the networkx usage in the ApproximateTokenSwapper to
use retworkx instead. Besides being speeding up the execution of the
pass what's more important here is that this was the last networkx usage
in qiskit-terra (with the exeception of a couple compatibility
converters in qiskit.dagcircuit). So this commit also makes networkx an
optional dependency and updates the 3 methods (which are outside the
common usage scenarios) to use runtime imports and emit a more detailed
import error if networkx isn't installed.

Details and comments

Fixes: #5470
Related to: #5100

This commit removes the networkx usage in the ApproximateTokenSwapper to
use retworkx instead. Besides being speeding up the execution of the
pass what's more important here is that this was the last networkx usage
in qiskit-terra (with the exeception of a couple compatibility
converters in qiskit.dagcircuit). So this commit also makes networkx an
optional dependency and updates the 3 methods (which are outside the
common usage scenarios) to use runtime imports and emit a more detailed
import error if networkx isn't installed.

Fixes: Qiskit#5470
Related to: Qiskit#5100
@mtreinish mtreinish requested a review from a team as a code owner December 4, 2020 19:19
@mtreinish mtreinish added Changelog: Removal Include in the Removed section of the changelog performance labels Dec 4, 2020
@mtreinish mtreinish added this to the 0.17 milestone Dec 4, 2020
@mtreinish mtreinish added this to To do in Transpiler via automation Dec 4, 2020
@mtreinish mtreinish changed the title Migrate ApproximateTokenSwapper to only use retworkx Migrate ApproximateTokenSwapper to only use retworkx and make networkx optional Dec 4, 2020
There is a single dagcircuit test that uses the to_networkx() method. So
we need to install networkx in test environments.
@eddieschoute
Copy link
Contributor

Cool, I see you narrowed the allowed types on the graph to integers. I assume that is because only integers are supported by retworkx?

@mtreinish
Copy link
Member Author

mtreinish commented Dec 4, 2020

Cool, I see you narrowed the allowed types on the graph to integers. I assume that is because only integers are supported by retworkx?

Sort of, retworkx graphs allow any python object as a node's data payload/weight, but all nodes are integer indexed, and unlike networkx, retworkx functions that take a node don't use an associative lookup based on the data payload and must be passed an index. I changed it to just be int because I have it work with only index when calling retworkx. We could change it to use anything but then we'd have to maintain a mapping of node data to node index somewhere (for dagcircuit we store the index in the dagnode, so it doesn't have to be a dict).

Co-authored-by: Lauren Capelluto <laurencapelluto@gmail.com>
Copy link
Contributor

@lcapelluto lcapelluto left a comment

Choose a reason for hiding this comment

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

Cool! 🎉

lcapelluto
lcapelluto previously approved these changes Dec 4, 2020
@mtreinish
Copy link
Member Author

mtreinish commented Dec 4, 2020

I ran some quick benchmarks of the token swapper just now:

create

The big improvement in creation time for retworkx is at 300 nodes, it's the default threshold for using the multiple threads: https://retworkx.readthedocs.io/en/stable/stubs/retworkx.graph_distance_matrix.html It might be worth tuning that a bit and setting it to a lower number.

map

(both graphs are log-log)

I generated the data with:

import time

import retworkx as rx
import numpy as np
from qiskit.transpiler.passes.routing.algorithms import ApproximateTokenSwapper

np.random.seed(42)
create_time = {}
map_time = {}

for size in np.logspace(2, 3, dtype=int):
    print(size)
    graph = rx.undirected_gnm_random_graph(size, size ** 2 // 10, seed=42)
    for i in graph.node_indexes():
        try:
            graph.remove_edge(i, i)  # Remove self-loops.
        except rx.NoEdgeBetweenNodes:
            continue
    # Make sure the graph is connected by adding C_n
    graph.add_edges_from_no_data(
        [(i, i + 1) for i in range(len(graph) - 1)])
    start = time.time()
    swapper = ApproximateTokenSwapper(graph)
    stop = time.time()
    create_time[size] = stop - start

    # Generate a randomized permutation.
    rand_perm = np.random.permutation(graph.nodes())
    permutation = dict(zip(graph.nodes(), rand_perm))
    mapping = dict(itertools.islice(permutation.items(), 0, size, 2))
    start = time.time()
    out = list(swapper.map(mapping, trials=40))
    stop = time.time()
    map_time[size] = stop - start

@ajavadia ajavadia merged commit 75e600e into Qiskit:master Dec 7, 2020
Transpiler automation moved this from To do to done Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Removal Include in the Removed section of the changelog performance
Projects
Transpiler
  
done
Development

Successfully merging this pull request may close these issues.

Migrate ApproximateTokenSwapper to retworkx
4 participants