Topological sort #24
Topological sort #24
Conversation
__tests__/topological_sort_test.re
Outdated
| test("forest", () => { | ||
|
|
||
| let adj_list = [ | ||
| {id: "A", neighbours: ["B"]}, |
There was a problem hiding this comment.
In topological sort, we are dealing with directed graphs. What's a better word for neighbors?
src/graphs/TopologicalSort.re
Outdated
|
|
||
| type directedGraph = list(node); | ||
|
|
||
| exception Not_found(string); |
There was a problem hiding this comment.
I don't think we need to redefine the Not_found exception since it's defined in the Pervasive module.
There was a problem hiding this comment.
I'm aware, I thought it would be more informative for the user if we provide the value of the node which wasn't found as well, which the Pervasives version of Not_found doesn't provide.
There was a problem hiding this comment.
What do you think, should I just use the Pervasives module exception?
src/graphs/TopologicalSort.re
Outdated
| neighbours: list(string), | ||
| }; | ||
|
|
||
| type mark = |
There was a problem hiding this comment.
can you explain how mark is used?
There was a problem hiding this comment.
So this is the alg I'm using from https://en.wikipedia.org/wiki/Topological_sorting:
mark stores the mark type for each node (temporary or permanant).
There was a problem hiding this comment.
right, the algorithm mentioned in the MIT 6006 uses finish times.
https://ocw.mit.edu/courses/electrical-engineering-and-computer-science/6-006-introduction-to-algorithms-fall-2011/lecture-videos/MIT6_006F11_lec14.pdf
Let's keep it consistent with their solution. So someone can watch the lecture and look at our implementation and just get it
There was a problem hiding this comment.
@selfeki I think we need a way to unify all the graph algorithms in a module. I don't like it all scattered where every function is in its own module.
There was a problem hiding this comment.
@alireza-a, "I think we need a way to unify all the graph algorithms in a module.". Could you elaborate a bit more? I'm a little confused.
src/graphs/TopologicalSort.re
Outdated
|
|
||
| let rec visit = (~node_id, ~adj_tbl, ~mark_tbl, ~list) => { | ||
| if (Hashtbl.mem(mark_tbl, node_id)) { | ||
| switch (Hashtbl.find(mark_tbl, node_id)) { |
There was a problem hiding this comment.
you can match the exception instead of nesting.
src/graphs/TopologicalSort.re
Outdated
| adj_tbl; | ||
| }; | ||
|
|
||
| let rec visit = (~node_id, ~adj_tbl, ~mark_tbl, ~list) => { |
There was a problem hiding this comment.
maybe a more descriptive name helps.
src/graphs/TopologicalSort.re
Outdated
| adj_tbl; | ||
| }; | ||
|
|
||
| let rec visit = (~node_id, ~adj_tbl, ~mark_tbl, ~list) => { |
There was a problem hiding this comment.
I'm a bit confused about what happens here. Can you add some comments explaining how visit works
|
@selfeki what's happening with this PR? |
| @@ -0,0 +1,8 @@ | |||
| type node = { | |||
There was a problem hiding this comment.
here we can use a type alias for string, type id = string and then we get
type node = {
id: id,
children: list(id)
}
...
let sort: directedGraph => list(id);
| ~ancestors=[]); | ||
| }; | ||
|
|
||
| let top_sorting = List.fold_left(traverse, [], adj_list); |
There was a problem hiding this comment.
what do you mean by top_sorting?
| let ancestors = List.append(ancestors, [node_id]); | ||
| let children = Hashtbl.find(adj_tbl, node_id); | ||
| let visitChild = (ordering, child_id) => { | ||
| visit( |
There was a problem hiding this comment.
I don't like this pattern here. Let's create a record type for the arguments to this function state. We can then create a new state by replacing only parts of the state like
state with node_id = child_id
| expect(sorting) |> toEqual(expected_sorting); | ||
| }); | ||
|
|
||
| test("forest", () => { |
There was a problem hiding this comment.
Can we add a test case similar to lecture 14 page 5? And make sure to visit B first?
2 participants
No description provided.