Better cycle detection #167
Conversation
|
This is ready for a first look. |
matthiasdiener
force-pushed
the
better-cycle
branch
3 times, most recently
from
cd96dc2
to
8a32515
Compare
matthiasdiener
force-pushed
the
better-cycle
branch
from
8a32515
to
27255a5
Compare
pytools/graph.py
Outdated
| """ | ||
| def dfs(node: NodeT, path: List[NodeT]) -> List[NodeT]: | ||
| # Cycle detected | ||
| if visited[node] == 1: |
There was a problem hiding this comment.
What do you think of a95c4c0?
| :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.
Constructing path just in case is wasteful IMO: The path could be collected as you return, if a cycle is found.
pytools/graph.py
Outdated
| res = [] | ||
|
|
||
| for node in graph: | ||
| if not visited[node]: | ||
| cycle = dfs(node, []) | ||
| if cycle: | ||
| res.append(cycle) |
There was a problem hiding this comment.
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.
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.
pytools/graph.py
Outdated
| @@ -240,6 +241,42 @@ def __init__(self, node: NodeT) -> None: | |||
| self.node = node | |||
|
|
|||
|
|
|||
| def find_cycles(graph: GraphT) -> List[List[NodeT]]: | |||
There was a problem hiding this comment.
Could use this in contains_cycle.
There was a problem hiding this comment.
Done in 29775e6
pytools/graph.py
Outdated
| """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.
Done in 18d241b
| raise CycleError(None) | ||
| else: | ||
| raise CycleError(cycles[0][0]) |
There was a problem hiding this comment.
Add to the documentation of CycleError what the value might mean.
There was a problem hiding this comment.
Hmm, the current doc of CycleError has :attr node: Node in a directed graph that is part of a cycle. - I'm not sure what else to add there.
matthiasdiener
force-pushed
the
better-cycle
branch
from
902e9ab
to
29775e6
Compare
| WHITE = 0 # Not visited yet | ||
| GREY = 1 # Currently visiting | ||
| BLACK = 2 # Done visiting |
There was a problem hiding this comment.
Why not use descriptive names for the node state?
There was a problem hiding this comment.
If you prefer, I can rename these, but I thought white/grey/black were standard labels in DFS (see e.g. http://www.cs.cmu.edu/afs/cs/academic/class/15750-s17/ScribeNotes/lecture9.pdf)
pytools/graph.py
Outdated
| @@ -240,6 +243,52 @@ def __init__(self, node: NodeT) -> None: | |||
| self.node = node | |||
|
|
|||
|
|
|||
| class NodeState(Enum): | |||
There was a problem hiding this comment.
| class NodeState(Enum): | |
| class _NodeState(Enum): |
There was a problem hiding this comment.
Thanks, done in 85a4a29.
I think these PRs are orthogonal to each other. This PR adds a new cycle detection function and uses it to correct the cycle reporting in |
|
This does not work correctly in all cases, so I've set it back to draft now- |
Fixes #165
(This is
supera bit clumsy, but I couldn't think of a better way to do this apart from rewriting the function to use DFS (for which I don't know if that would work with a keyfunc) ...)