From a519347e75cb6a05cb3c88740921b491c32a5b16 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Wed, 7 Dec 2022 22:52:06 -0600 Subject: [PATCH 01/14] Better cycle detection --- pytools/graph.py | 23 ++++++++++++++++++++--- test/test_graph_tools.py | 19 +++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index 7f227bc6..c815f3fa 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -299,9 +299,26 @@ def compute_topological_order(graph: Mapping[T, Collection[T]], 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 + try: + validate_graph(graph) + except ValueError: + # Graph is invalid, we can't compute SCCs or return a meaningful node + # that is part of a cycle + raise CycleError(None) + + sccs = compute_sccs(graph) + cycles = [scc for scc in sccs if len(scc) > 1] + + if cycles: + # Cycles that are not self-loops + node = cycles[0][0] + else: + # Self-loop SCCs also have a length of 1 + node = next(iter(n for n, num_preds in + nodes_to_num_predecessors.items() if num_preds != 0)) + + raise CycleError(node) return order diff --git a/test/test_graph_tools.py b/test/test_graph_tools.py index 2f14f1ad..a166085e 100644 --- a/test/test_graph_tools.py +++ b/test/test_graph_tools.py @@ -395,6 +395,25 @@ def test_is_connected(): assert is_connected({}) +def test_cycle_detection(): + from pytools.graph import compute_topological_order, CycleError + + # Non-Self Loop + graph = {1: {}, 5: {1, 8}, 8: {5}} + with pytest.raises(CycleError, match="5"): + compute_topological_order(graph) + + # Self-Loop + graph = {1: {1}, 5: {8}, 8: {}} + with pytest.raises(CycleError, match="1"): + compute_topological_order(graph) + + # Invalid graph with loop + graph = {1: {42}, 5: {8}, 8: {5}} + with pytest.raises(CycleError, match="None"): + compute_topological_order(graph) + + if __name__ == "__main__": if len(sys.argv) > 1: exec(sys.argv[1]) From 2a6d9aecc16c01cca23f058353444ccde7827f9c Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Wed, 7 Dec 2022 22:55:10 -0600 Subject: [PATCH 02/14] make the non-self test a bit looser --- test/test_graph_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_graph_tools.py b/test/test_graph_tools.py index a166085e..299c7894 100644 --- a/test/test_graph_tools.py +++ b/test/test_graph_tools.py @@ -400,7 +400,7 @@ def test_cycle_detection(): # Non-Self Loop graph = {1: {}, 5: {1, 8}, 8: {5}} - with pytest.raises(CycleError, match="5"): + with pytest.raises(CycleError, match="5|8"): compute_topological_order(graph) # Self-Loop From b9ac7b8755b5556bd40be36cf31c836ec99fbaff Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Fri, 9 Dec 2022 11:17:25 -0600 Subject: [PATCH 03/14] make verbose cycle detection optional --- pytools/graph.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index c815f3fa..32ac8831 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -236,7 +236,8 @@ def __lt__(self, other: "HeapEntry") -> bool: def compute_topological_order(graph: Mapping[T, Collection[T]], - key: Optional[Callable[[T], Any]] = None) -> List[T]: + key: Optional[Callable[[T], Any]] = None, + verbose_cycle: bool = True) -> List[T]: """Compute a topological order of nodes in a directed graph. :arg graph: A :class:`collections.abc.Mapping` representing a directed @@ -248,6 +249,8 @@ def compute_topological_order(graph: Mapping[T, Collection[T]], 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. + :returns: A :class:`list` representing a valid topological ordering of the nodes in the directed graph. @@ -300,6 +303,9 @@ def compute_topological_order(graph: Mapping[T, Collection[T]], if len(order) != total_num_nodes: # There is a cycle in the graph + if not verbose_cycle: + raise CycleError(None) + try: validate_graph(graph) except ValueError: @@ -377,7 +383,7 @@ def contains_cycle(graph: Mapping[T, Collection[T]]) -> bool: """ try: - compute_topological_order(graph) + compute_topological_order(graph, verbose_cycle=False) return False except CycleError: return True From 27255a589fc4d77deabd3463a81f52d603cffb22 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Mon, 12 Dec 2022 08:40:35 -0600 Subject: [PATCH 04/14] add find_cycles and use it --- pytools/graph.py | 61 ++++++++++++++++++++++++++++------------ test/test_graph_tools.py | 13 +++++++-- 2 files changed, 54 insertions(+), 20 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index 824506b7..bce8d740 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -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]: + # Cycle detected + if visited[node] == 1: + 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) + + return res + + class HeapEntry: """ Helper class to compare associated keys while comparing the elements in @@ -257,8 +294,8 @@ def __lt__(self, other: "HeapEntry") -> bool: def compute_topological_order(graph: GraphT, - key: Optional[Callable[[T], Any]] = None, - verbose_cycle: bool = True) -> List[T]: + 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 @@ -323,24 +360,12 @@ def compute_topological_order(graph: GraphT, raise CycleError(None) try: - validate_graph(graph) - except ValueError: - # Graph is invalid, we can't compute SCCs or return a meaningful node - # that is part of a cycle + cycles = find_cycles(graph) + except KeyError: + # Graph is invalid raise CycleError(None) - - sccs = compute_sccs(graph) - cycles = [scc for scc in sccs if len(scc) > 1] - - if cycles: - # Cycles that are not self-loops - node = cycles[0][0] else: - # Self-loop SCCs also have a length of 1 - node = next(iter(n for n, num_preds in - nodes_to_num_predecessors.items() if num_preds != 0)) - - raise CycleError(node) + raise CycleError(cycles[0][0]) return order diff --git a/test/test_graph_tools.py b/test/test_graph_tools.py index 299c7894..115124e5 100644 --- a/test/test_graph_tools.py +++ b/test/test_graph_tools.py @@ -395,24 +395,33 @@ def test_is_connected(): assert is_connected({}) -def test_cycle_detection(): - from pytools.graph import compute_topological_order, CycleError +def test_find_cycles(): + from pytools.graph import compute_topological_order, CycleError, find_cycles # Non-Self Loop graph = {1: {}, 5: {1, 8}, 8: {5}} + assert find_cycles(graph) == [[5, 8]] with pytest.raises(CycleError, match="5|8"): compute_topological_order(graph) # Self-Loop graph = {1: {1}, 5: {8}, 8: {}} + assert find_cycles(graph) == [[1]] with pytest.raises(CycleError, match="1"): compute_topological_order(graph) # Invalid graph with loop graph = {1: {42}, 5: {8}, 8: {5}} + # Can't run find_cycles on this graph since it is invalid with pytest.raises(CycleError, match="None"): compute_topological_order(graph) + # Multiple loops + graph = {1: {1}, 5: {8}, 8: {5}} + assert find_cycles(graph) == [[1], [5, 8]] + with pytest.raises(CycleError, match="1"): + compute_topological_order(graph) + if __name__ == "__main__": if len(sys.argv) > 1: From 29775e693bbe52a5e347998fd4348fe988373f17 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Mon, 12 Dec 2022 11:57:30 -0600 Subject: [PATCH 05/14] use find_cycles in contains_cycle --- pytools/graph.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index bce8d740..f79adc23 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -417,11 +417,7 @@ def contains_cycle(graph: GraphT) -> bool: .. versionadded:: 2020.2 """ - try: - compute_topological_order(graph, verbose_cycle=False) - return False - except CycleError: - return True + return bool(find_cycles(graph)) # }}} From 2ff7defc670990b2809770217c2c608f5d59e0e5 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Mon, 12 Dec 2022 12:02:50 -0600 Subject: [PATCH 06/14] make finding all cycles optional --- pytools/graph.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index f79adc23..8ddfd0a7 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -241,10 +241,12 @@ def __init__(self, node: NodeT) -> None: self.node = node -def find_cycles(graph: GraphT) -> List[List[NodeT]]: +def find_cycles(graph: GraphT, all_cycles: bool = True) -> List[List[NodeT]]: """ Find all cycles in *graph* using DFS. + :arg all_cycles: If False, only return the first cycle found. + :returns: A :class:`list` in which each element represents another :class:`list` of nodes that form a cycle. """ @@ -273,6 +275,8 @@ def dfs(node: NodeT, path: List[NodeT]) -> List[NodeT]: cycle = dfs(node, []) if cycle: res.append(cycle) + if not all_cycles: + return res return res @@ -417,7 +421,7 @@ def contains_cycle(graph: GraphT) -> bool: .. versionadded:: 2020.2 """ - return bool(find_cycles(graph)) + return bool(find_cycles(graph, all_cycles=False)) # }}} From 18d241b709ba55c19d87176c89cad14f9af0024e Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Mon, 12 Dec 2022 12:08:00 -0600 Subject: [PATCH 07/14] better verbose_cycle doc --- pytools/graph.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pytools/graph.py b/pytools/graph.py index 8ddfd0a7..14ff2ba6 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -306,7 +306,8 @@ def compute_topological_order(graph: GraphT, 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. + :arg verbose_cycle: Verbose reporting in case *graph* contains a cycle, i.e. + return a :class:`CycleError` which has a node that is part of a cycle. :returns: A :class:`list` representing a valid topological ordering of the nodes in the directed graph. From a95c4c0bd0c6caa64906542d7f5c9ad81e1815c7 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Mon, 12 Dec 2022 12:18:34 -0600 Subject: [PATCH 08/14] use enum for node states --- pytools/graph.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index 14ff2ba6..4ac6b03d 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -67,6 +67,8 @@ Callable, Set, MutableSet, Dict, Iterator, Tuple, Hashable) +from enum import Enum + try: from typing import TypeAlias except ImportError: @@ -241,6 +243,12 @@ def __init__(self, node: NodeT) -> None: self.node = node +class NodeState(Enum): + WHITE = 0 # Not visited yet + GREY = 1 # Currently visiting + BLACK = 2 # Done visiting + + def find_cycles(graph: GraphT, all_cycles: bool = True) -> List[List[NodeT]]: """ Find all cycles in *graph* using DFS. @@ -252,26 +260,26 @@ def find_cycles(graph: GraphT, all_cycles: bool = True) -> List[List[NodeT]]: """ def dfs(node: NodeT, path: List[NodeT]) -> List[NodeT]: # Cycle detected - if visited[node] == 1: + if visited[node] == NodeState.GREY: return path # Visit this node, explore its children - visited[node] = 1 + visited[node] = NodeState.GREY path.append(node) for child in graph[node]: - if visited[child] != 2 and dfs(child, path): + if visited[child] != NodeState.BLACK and dfs(child, path): return path # Done visiting node - visited[node] = 2 + visited[node] = NodeState.BLACK return [] - visited = {node: 0 for node in graph.keys()} + visited = {node: NodeState.WHITE for node in graph.keys()} res = [] for node in graph: - if not visited[node]: + if visited[node] == NodeState.WHITE: cycle = dfs(node, []) if cycle: res.append(cycle) From b5d3af60b29c7c267623a7415213aeb961d7a765 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Mon, 12 Dec 2022 12:29:21 -0600 Subject: [PATCH 09/14] adjust doc --- pytools/graph.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pytools/graph.py b/pytools/graph.py index 4ac6b03d..134da073 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -251,7 +251,7 @@ class NodeState(Enum): def find_cycles(graph: GraphT, all_cycles: bool = True) -> List[List[NodeT]]: """ - Find all cycles in *graph* using DFS. + Find cycles in *graph* using DFS. :arg all_cycles: If False, only return the first cycle found. From 96bd3a66e13aca8adcdb8cdee63965dd2bba15f9 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Fri, 16 Dec 2022 15:46:05 +0100 Subject: [PATCH 10/14] make path storage more lean --- pytools/graph.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index 134da073..4bb21902 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -261,14 +261,17 @@ def find_cycles(graph: GraphT, all_cycles: bool = True) -> List[List[NodeT]]: def dfs(node: NodeT, path: List[NodeT]) -> List[NodeT]: # Cycle detected if visited[node] == NodeState.GREY: - return path + return path + [node] # Visit this node, explore its children visited[node] = NodeState.GREY - path.append(node) + tmp = [] for child in graph[node]: + if child != node: + # Don't duplicate nodes for self-cycles + tmp.append(child) if visited[child] != NodeState.BLACK and dfs(child, path): - return path + return path + [node] + tmp # Done visiting node visited[node] = NodeState.BLACK From e0ba463e5ff86ca0690a5322ebf49eb419b48990 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Fri, 16 Dec 2022 15:51:49 +0100 Subject: [PATCH 11/14] mypy --- pytools/graph.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pytools/graph.py b/pytools/graph.py index a8f5fa37..dad4a2b5 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -376,7 +376,7 @@ def compute_topological_order(graph: GraphT[NodeT], raise CycleError(None) try: - cycles = find_cycles(graph) + cycles: List[List[NodeT]] = find_cycles(graph) except KeyError: # Graph is invalid raise CycleError(None) From 3640c88f845168e2cfb32749240304e7681cb7ae Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Fri, 16 Dec 2022 15:51:49 +0100 Subject: [PATCH 12/14] fix --- pytools/graph.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index dad4a2b5..910243f5 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -265,13 +265,10 @@ def dfs(node: NodeT, path: List[NodeT]) -> List[NodeT]: # Visit this node, explore its children visited[node] = NodeState.GREY - tmp = [] for child in graph[node]: - if child != node: - # Don't duplicate nodes for self-cycles - tmp.append(child) if visited[child] != NodeState.BLACK and dfs(child, path): - return path + [node] + tmp + return path + [node] + ( + [child] if child != node else []) # Done visiting node visited[node] = NodeState.BLACK From 85a4a29526c0bbe5763f60e67018196510d21433 Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Sun, 25 Dec 2022 10:11:58 +0100 Subject: [PATCH 13/14] rename to _NodeState --- pytools/graph.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pytools/graph.py b/pytools/graph.py index 910243f5..375652ad 100644 --- a/pytools/graph.py +++ b/pytools/graph.py @@ -243,7 +243,7 @@ def __init__(self, node: NodeT) -> None: self.node = node -class NodeState(Enum): +class _NodeState(Enum): WHITE = 0 # Not visited yet GREY = 1 # Currently visiting BLACK = 2 # Done visiting @@ -260,26 +260,26 @@ def find_cycles(graph: GraphT, all_cycles: bool = True) -> List[List[NodeT]]: """ def dfs(node: NodeT, path: List[NodeT]) -> List[NodeT]: # Cycle detected - if visited[node] == NodeState.GREY: + if visited[node] == _NodeState.GREY: return path + [node] # Visit this node, explore its children - visited[node] = NodeState.GREY + visited[node] = _NodeState.GREY for child in graph[node]: - if visited[child] != NodeState.BLACK and dfs(child, path): + if visited[child] != _NodeState.BLACK and dfs(child, path): return path + [node] + ( [child] if child != node else []) # Done visiting node - visited[node] = NodeState.BLACK + visited[node] = _NodeState.BLACK return [] - visited = {node: NodeState.WHITE for node in graph.keys()} + visited = {node: _NodeState.WHITE for node in graph.keys()} res = [] for node in graph: - if visited[node] == NodeState.WHITE: + if visited[node] == _NodeState.WHITE: cycle = dfs(node, []) if cycle: res.append(cycle) From 67f948e49293136730824eeec2c0b0d620c6e46d Mon Sep 17 00:00:00 2001 From: Matthias Diener Date: Sun, 8 Jan 2023 16:43:19 +0100 Subject: [PATCH 14/14] add failing test --- test/test_graph_tools.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test_graph_tools.py b/test/test_graph_tools.py index f9c3b43b..30bbcb8d 100644 --- a/test/test_graph_tools.py +++ b/test/test_graph_tools.py @@ -455,6 +455,10 @@ def test_find_cycles(): with pytest.raises(CycleError, match="1"): compute_topological_order(graph) + # Cycle over multiple nodes + graph = {4: {2}, 2: {3}, 3: {4}} + assert find_cycles(graph) == [[4, 2, 3]] + if __name__ == "__main__": if len(sys.argv) > 1: