-
-
Notifications
You must be signed in to change notification settings - Fork 50.5k
Fix topological sort order #14609
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
base: master
Are you sure you want to change the base?
Fix topological sort order #14609
Changes from all commits
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 |
|---|---|---|
|
|
@@ -15,27 +15,53 @@ | |
| vertices: list[str] = ["a", "b", "c", "d", "e"] | ||
|
|
||
|
|
||
| def topological_sort(start: str, visited: list[str], sort: list[str]) -> list[str]: | ||
| """Perform topological sort on a directed acyclic graph.""" | ||
| current = start | ||
| # add current to visited | ||
| def _visit( | ||
| current: str, | ||
| visited: list[str], | ||
| post_order: list[str], | ||
| graph: dict[str, list[str]], | ||
| ) -> None: | ||
| """Visit all descendants of the current vertex using DFS.""" | ||
| visited.append(current) | ||
| neighbors = edges[current] | ||
| for neighbor in neighbors: | ||
| # if neighbor not in visited, visit | ||
| for neighbor in graph[current]: | ||
| if neighbor not in visited: | ||
| sort = topological_sort(neighbor, visited, sort) | ||
| # if all neighbors visited add current to sort | ||
| sort.append(current) | ||
| # if all vertices haven't been visited select a new one to visit | ||
| if len(visited) != len(vertices): | ||
| for vertice in vertices: | ||
| _visit(neighbor, visited, post_order, graph) | ||
| post_order.append(current) | ||
|
|
||
|
|
||
| def topological_sort( | ||
| start: str, | ||
| visited: list[str], | ||
| sort: list[str], | ||
| graph: dict[str, list[str]] | None = None, | ||
|
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. Using mutable default arguments as None with an explicit check on However the parameter name "sort" on line 35 is a bit confusing def topological_sort( |
||
| vertices_list: list[str] | None = None, | ||
| ) -> list[str]: | ||
| """ | ||
| Perform topological sort on a directed acyclic graph. | ||
|
|
||
| >>> result = topological_sort("a", [], [], edges, vertices) | ||
| >>> all( | ||
| ... result.index(parent) < result.index(child) | ||
| ... for parent, children in edges.items() | ||
| ... for child in children | ||
| ... ) | ||
| True | ||
| """ | ||
| if graph is None: | ||
| graph = edges | ||
| if vertices_list is None: | ||
| vertices_list = list(graph) | ||
|
|
||
| _visit(start, visited, sort, graph) | ||
| if len(visited) != len(vertices_list): | ||
| for vertice in vertices_list: | ||
| if vertice not in visited: | ||
| sort = topological_sort(vertice, visited, sort) | ||
| # return sort | ||
| _visit(vertice, visited, sort, graph) | ||
| sort.reverse() | ||
|
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. This is the core fix — adding sort.reverse() correctly produces Worth adding a brief comment explaining why the reverse is needed Post-order DFS gives reverse topological order, so reverse to fixsort.reverse() |
||
| return sort | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sort = topological_sort("a", [], []) | ||
| print(sort) | ||
| result = topological_sort("a", [], [], edges, vertices) | ||
| assert result == ["a", "b", "e", "d", "c"] | ||
| print(result) | ||
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.
Good refactor extracting _visit() as a private helper — separating
the DFS traversal from the main topological_sort() logic makes both
functions easier to understand and test independently.
One suggestion: _visit() modifies visited and post_order in place
via side effects but has no doctest. Since it's a private helper
this is acceptable, but a one-line docstring explaining the side
effects would help:
def _visit(current, visited, post_order, graph):
"""DFS helper — appends nodes to visited and post_order in place."""