-
Notifications
You must be signed in to change notification settings - Fork 25
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
Better cycle detection #167
base: main
Are you sure you want to change the base?
Changes from 5 commits
a519347
2a6d9ae
b9ac7b8
e9d1f57
27255a5
29775e6
2ff7def
18d241b
a95c4c0
b5d3af6
96bd3a6
080ff03
e0ba463
3640c88
85a4a29
dca95b9
67f948e
7e79f5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,7 @@ | |
.. autoexception:: CycleError | ||
.. autofunction:: compute_topological_order | ||
.. autofunction:: compute_transitive_closure | ||
.. autofunction:: find_cycles | ||
.. autofunction:: contains_cycle | ||
.. autofunction:: compute_induced_subgraph | ||
.. autofunction:: validate_graph | ||
|
@@ -240,6 +241,42 @@ def __init__(self, node: NodeT) -> None: | |
self.node = node | ||
|
||
|
||
def find_cycles(graph: GraphT) -> List[List[NodeT]]: | ||
""" | ||
Find all cycles in *graph* using DFS. | ||
|
||
:returns: A :class:`list` in which each element represents another :class:`list` | ||
of nodes that form a cycle. | ||
""" | ||
def dfs(node: NodeT, path: List[NodeT]) -> List[NodeT]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Constructing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
# Cycle detected | ||
if visited[node] == 1: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make 0, 1, 2 an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of a95c4c0? |
||
return path | ||
|
||
# Visit this node, explore its children | ||
visited[node] = 1 | ||
path.append(node) | ||
for child in graph[node]: | ||
if visited[child] != 2 and dfs(child, path): | ||
return path | ||
|
||
# Done visiting node | ||
visited[node] = 2 | ||
return [] | ||
|
||
visited = {node: 0 for node in graph.keys()} | ||
|
||
res = [] | ||
|
||
for node in graph: | ||
if not visited[node]: | ||
cycle = dfs(node, []) | ||
if cycle: | ||
res.append(cycle) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding all cycles is probably too much... all the use cases I can think of will only want one. I could see this as an option, maybe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made it an optional parameter in 2ff7def. My thinking was that for the use case I had in mind, checking the validity of our DAG in pytato, having all cycles instead of just one may be beneficial. |
||
|
||
return res | ||
|
||
|
||
class HeapEntry: | ||
""" | ||
Helper class to compare associated keys while comparing the elements in | ||
|
@@ -257,14 +294,16 @@ def __lt__(self, other: "HeapEntry") -> bool: | |
|
||
|
||
def compute_topological_order(graph: GraphT, | ||
key: Optional[Callable[[NodeT], Any]] = None) \ | ||
-> List[NodeT]: | ||
key: Optional[Callable[[NodeT], Any]] = None, | ||
verbose_cycle: bool = True) -> List[NodeT]: | ||
"""Compute a topological order of nodes in a directed graph. | ||
|
||
:arg key: A custom key function may be supplied to determine the order in | ||
break-even cases. Expects a function of one argument that is used to | ||
extract a comparison key from each node of the *graph*. | ||
|
||
:arg verbose_cycle: Verbose reporting in case *graph* contains a cycle. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Say what this means. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 18d241b |
||
|
||
:returns: A :class:`list` representing a valid topological ordering of the | ||
nodes in the directed graph. | ||
|
||
|
@@ -316,9 +355,17 @@ def compute_topological_order(graph: GraphT, | |
heappush(heap, HeapEntry(child, keyfunc(child))) | ||
|
||
if len(order) != total_num_nodes: | ||
# any node which has a predecessor left is a part of a cycle | ||
raise CycleError(next(iter(n for n, num_preds in | ||
nodes_to_num_predecessors.items() if num_preds != 0))) | ||
# There is a cycle in the graph | ||
inducer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if not verbose_cycle: | ||
raise CycleError(None) | ||
|
||
try: | ||
cycles = find_cycles(graph) | ||
except KeyError: | ||
# Graph is invalid | ||
raise CycleError(None) | ||
else: | ||
raise CycleError(cycles[0][0]) | ||
Comment on lines
+381
to
+383
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add to the documentation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, the current doc of |
||
|
||
return order | ||
|
||
|
@@ -371,7 +418,7 @@ def contains_cycle(graph: GraphT) -> bool: | |
""" | ||
|
||
try: | ||
compute_topological_order(graph) | ||
compute_topological_order(graph, verbose_cycle=False) | ||
return False | ||
except CycleError: | ||
return True | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use this in
contains_cycle
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 29775e6