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

Improved performance of topological_sort and topological_sort_recursive. #763

Merged
merged 1 commit into from
Sep 9, 2012

Conversation

larose
Copy link
Contributor

@larose larose commented Sep 9, 2012

The current implementations of topological_sort and topological_sort_recursive are slow because they do not take into consideration how list is implemented in CPython (it is implemented as an array).

Instead of inserting at the beginning of a list, it is preferable to insert at the end (append) and then reverse the list once at the end of the algorithm.

Moreover, "in" on a list takes O(N) as opposed to O(1) on a set.

On a randomly generated dag with 199496 vertices and 600000 arcs, topological_sort took ~15 seconds and topological_sort_recursive took > 15 minutes. Now they take less than a second. See https://github.com/larose/nx-topological-sort-benchmark

topological_sort
   - Replaced list.insert with list.append
   - Replaced dict with set

topological_sort_recursive
   - Replaced list.insert with list.append
   - Replaced a linear search with a constant time search.
@ghost ghost assigned hagberg Sep 9, 2012
@hagberg
Copy link
Member

hagberg commented Sep 9, 2012

Great, thanks! Good idea to use append() rather than insert().
And I think the original algorithm was written before set() existed in Python...

If this passes our tests we can merge it ASAP.

@chebee7i
Copy link
Member

chebee7i commented Sep 9, 2012

All tests pass for me. I'll go ahead and merge it. Probably, there are other algorithms where similar improvements could be achieved, especially with respect to the "in" operation. Also, I will add a Wiki page which shows how to run tests on a pull request.

chebee7i added a commit that referenced this pull request Sep 9, 2012
Improved performance of topological_sort and topological_sort_recursive.
@chebee7i chebee7i merged commit 569928a into networkx:master Sep 9, 2012
@chebee7i
Copy link
Member

chebee7i commented Sep 9, 2012

Actually, it doesn't look like the wiki is enabled for the NetworkX repository. Where should I put this information?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants