[WIP] Add topological sort algorithm#5492
[WIP] Add topological sort algorithm#5492ngokulakrish wants to merge 4 commits intorapidsai:mainfrom
Conversation
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.
| }); | ||
|
|
||
| frontier_vertices = std::move(new_frontier_vertices); | ||
| level++; |
There was a problem hiding this comment.
Have you checked (with a small toy graph) what happens if the input graph has a cycle and do_expensive_check is set to false?
I guess in_degrees can have negative values and either we exits the loop pre-maturely or we fall into infinite loops.
We may accumulate the total aggregate frontier size and we may be able to detect pre-mature exit (if total < # vertices in the graph) or infinite loops (total > # vertices in the graph).
If there is no way to cheaply check this, we update documentation. Throws if the graph is symmetric or has cycles AND do_expensive_check is set to true. Otherwise, undefined behavior. If we can cheaply check this, we should check.
This PR is to add topological sort algorithm to cuGraph.