Add topological sort algorithm#5492
Conversation
seunghwak
added
feature request
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
Looks great except for minor cosmetic issues! We need to add tests and work on merging this.
| bool do_expensive_check = false); | ||
|
|
||
| /** | ||
| * @ingroup traversal_cpp |
There was a problem hiding this comment.
This doesn't belong to the traversal group.
https://github.com/rapidsai/cugraph/blob/main/cpp/include/cugraph/algorithms.hpp#L31
We may create dag_group here. That place this algorithm in the dag_group.
@ChuckHastings @acostadon @alexbarghi-nv @rlratzel @BradReesWork Any thoughts on where should we place topological sort?
In networkX, topological sort is in dag.py (https://github.com/networkx/networkx/blob/main/networkx/algorithms/dag.py) along with
__all__ = [
"descendants",
"ancestors",
"topological_sort",
"lexicographical_topological_sort",
"all_topological_sorts",
"topological_generations",
"is_directed_acyclic_graph",
"is_aperiodic",
"transitive_closure",
"transitive_closure_dag",
"transitive_reduction",
"antichains",
"dag_longest_path",
"dag_longest_path_length",
"dag_to_branching",
]
So, we initially placed this algorithm in the dag category.
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
Looks good and let's address few minor cosmetic issues; add a mechanism to avoid undesirable behaviors (possibly infinite looping) when user passed a graph with cycles (and set do_expensive_check = true); and finish SG/MG C++ tests (and we need to remove loops before passing the graph to topological_sort.
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
Looks good except for few minor cosmetic issues (+ you need to clear the edge mask before attaching a new one if the graph already has an attached edge mask).
Once you make these updates, I will test with 2 GPUs, and we can work on merging this.
seunghwak
changed the title
|
/ok to test 2115518 |
|
/ok to test 8887497 |
|
/merge |
3 participants
This PR is to add topological sort algorithm to cuGraph.