Find the longest path to each node from a starting node #161
Conversation
| @@ -0,0 +1,40 @@ | |||
| import networkx as nx | |||
There was a problem hiding this comment.
Module should have a docstring
There was a problem hiding this comment.
Also, according to PEP8, Python standard library imports should come before 3rd part library imports. Please swap these two and add a blank line in between.
conda_forge_tick/path_lengths.py
Outdated
| from copy import deepcopy | ||
|
|
||
|
|
||
| def cyclic_topological_sort(graph, source): |
There was a problem hiding this comment.
Also, all public functions should have docstrings.
|
Thanks for putting this in @justcalamari! This would be a prime candiate for adding some unit tests since it is so self-contained. |
conda_forge_tick/path_lengths.py
Outdated
| g2.remove_node(node) | ||
|
|
||
| dist = get_longest_paths(g2, source) | ||
| return {v: [k for k in dist if dist[k] == v] for v in dist.values()} |
There was a problem hiding this comment.
While this is perhaps the most readable nested comprehension I have ever seen, there are a couple of more efficient methods. For example, with a defaultdict you can only have one loop.
from collections import defaultdict
rtn = defaultdict(list)
for k, v in dist.items():
rtn[v].append(k)
return rtnOr you can dive into using itertools.groupby() but I am not sure it will make the code more readable
|
@scopatz Ready for review |
|
Looks great, I think it just needs tests |
|
@scopatz Ready for review |
conda_forge_tick/path_lengths.py
Outdated
| return dist | ||
|
|
||
|
|
||
| def get_levels(graph_file, source): |
There was a problem hiding this comment.
Should this take in a graph rather than a file?
There was a problem hiding this comment.
Yeah that's probably a good idea.
No description provided.