Dijkstra's shortest path #26
Conversation
| @@ -0,0 +1,19 @@ | |||
| type node = { | |||
| id: string, | |||
| neighbours: list(string), | |||
There was a problem hiding this comment.
Does a node need a set of neighbors? We can calculate them based on the edges.
| weight: int, | ||
| }; | ||
|
|
||
| type directedGraph = list(node); |
There was a problem hiding this comment.
why are you passing in the edges outside of the directedGraph?
There was a problem hiding this comment.
why don't we only specify the edges and construct the nodes inside the module?
There was a problem hiding this comment.
What if a node does not have any incident edges? I think we still need a list of nodes but without the neighbours field.
|
|
||
| let infinity = Pervasives.max_int; | ||
|
|
||
| let parseAdjList = adj_list => { |
There was a problem hiding this comment.
I think we should construct the adj_table from the edges instead. We don't need to specify the adj_list at all!
| Hashtbl.add(weight_tbl, id, adj_edge_weights); | ||
| }; | ||
|
|
||
| List.iter(initializeWeights, adj_list); |
There was a problem hiding this comment.
why creating a nested Hash here? Why not using a tuple as the key?
| }; | ||
|
|
||
| let add = (a, b) => { | ||
| if (a == infinity || b == infinity) { |
There was a problem hiding this comment.
I don't think we should use infinity. Pervasive.infinity is for floating points but here we are dealing with int. The distance hash_tbl can map to option(int)
There was a problem hiding this comment.
I used let infinity = Pervasives.max_int; not let infinity = Pervasives.infinity.
So should I still change this?
There was a problem hiding this comment.
debatable but i think it's fine.
| }; | ||
| }; | ||
|
|
||
| let rec traverse = (~queue, ~adj_tbl, ~weight_tbl, ~dist, ~prev) => { |
There was a problem hiding this comment.
I think queue should be renamed to frontier?
| let edge_weight = getEdgeWeight(weight_tbl, current_id, neighbour_id); | ||
| let alt_dist = add(current_dist, edge_weight); | ||
|
|
||
| if (alt_dist < neighbour_dist) { |
There was a problem hiding this comment.
maybe we should refactor this and call it relax?
There was a problem hiding this comment.
You mean relax instead of alt_dist?
There was a problem hiding this comment.
my comment was for the content of the if statement
alireza-a
left a comment
alireza-a
left a comment
There was a problem hiding this comment.
A graph is a list of nodes and edges. Since we are dealing with a directed graph and all the edges have weights, we can simplify our interface by only requiring a list of node id's (when there is no edge to or from a node) and edge weight list (from, to, weight).
Using the weights we can create a set of helper functions for Adj, Weight ...
| if (alt_dist < neighbour_dist) { | ||
| Hashtbl.replace(dist, neighbour_id, alt_dist); | ||
| Hashtbl.replace(prev, neighbour_id, Some(current_id)); | ||
| Heap.remove(queue, (key, _) => key == neighbour_id) |> ignore; |
There was a problem hiding this comment.
should we add update function to the Heap?
There was a problem hiding this comment.
I added an update function to the Heap
2 participants
Adds Dijkstra's shortest path algorithm with unit tests.