all_shortest_paths function
#1017
Conversation
|
|
lucasvanmol
changed the title
all_shortest_paths function
There was a problem hiding this comment.
Thanks for opening your first PR! Don't forget to sign the CLA and I'm looking forward to reviewing your work.
Some thoughts on this early draft. I don't think the approach you chose is the best way to implement this.
all_shortest_paths can return an exponential number of paths, hence it needs to be a separate function. It can be possibly much slower, which would impact existing users of our Dijkstra function.
Another detail is that currently the code computes a lot of paths that are not necessary. The simplest way to find if an edge (u, v) is in a shortest path from s to t is to check if d(s, u) + w(u, v) + d(v, t) == d(s, t). You can perform two shortest path calls, one in the graph and another in the graph with reversed direction. Then, you could write either a backtracking code or iteration-based code to generate all shortest paths.
Pseudo-code for the best way to implement would be along the lines:
def all_shortest_paths(graph, s, t):
d_fwrd = dijkstra(graph, s)
d_bckwrd = dijkstra(graph.reverse(), t)
w = weights(graph)
shortest_paths = [] # list containing the answer
current_paths = [[s]] #541
for k in range(len(graph) - 1):
next_paths = []
for path in current_paths:
u = path[-1]
for v in neighbors(u):
if d_fwrd[u] + w[u][v] + d_bckwrd[v] == d_fwrd[t]:
new_path = path + [v]
if v == t:
shortest_pathd.append(new_path)
else:
next_paths.append(new_path)
current_paths = next_paths
return shortest_paths
There are some optimizations that can be done such as filtering the edges in advance to only include edges that are in at least one shortest path, using persistent data structures to avoid copying the array, etc.
|
Hi, thanks for the valuable and detailed feedback. I've updated the PR with your feedback in mind, however I haven't implemented the
|
| F: FnMut(G::EdgeRef) -> Result<K, E>, | ||
| K: Measure + Copy, | ||
| { | ||
| let scores: DictMap<G::NodeId, K> = dijkstra(&graph, start, Some(goal), &mut edge_cost, None)?; |
There was a problem hiding this comment.
Don't pass Some(goal). Passing that argument can trigger an early termination that leaves scores unfilled
There was a problem hiding this comment.
Looking briefly at the code and performing some (not quite rigorous) tests, it was my understanding that only scores that aren't part of any shortest path will be left unfilled. I'll try to come up with a proper proof and come back to you
| if !scores.contains_key(&goal) { | ||
| return Ok(HashSet::default()); | ||
| } | ||
| let mut paths = HashSet::new(); |
There was a problem hiding this comment.
This can just be a list
Also, the return type should be just a list of lists, like |
| F: FnMut(G::EdgeRef) -> Result<K, E>, | ||
| K: Measure + Copy, | ||
| { | ||
| let scores: DictMap<G::NodeId, K> = dijkstra(&graph, start, Some(goal), &mut edge_cost, None)?; |
There was a problem hiding this comment.
Looking briefly at the code and performing some (not quite rigorous) tests, it was my understanding that only scores that aren't part of any shortest path will be left unfilled. I'll try to come up with a proper proof and come back to you
With regards to the scores: it’s a valid assumption if the weights are positive. We don’t support negative weight in Dijkstra. However, you need to keep in mind that there can be edges with weight zero. It’s worth adding this test case: The code could halt and never calculate a distance for C. But the correct answers are A->B and A->C->B |
|
Indeed, the following graph doesn't explore all the nodes of all shortest paths: graph LR;
a((a)) -->|1| b((b)) ---->|2| f((f));
a((a)) -->|2| c((c)) -->|1| d((d)) -->|0| e((e)) -->|0| f((f));
The path However, I'm not sure what the best way to fix this is because this means graphs can be created using 0 weight cycles which can create infinite paths. I see networkx only returns simple paths with no repeated nodes, so it's pretty easy to do the same. Hence using However, I think there could be a substantial performance increase for using a version of the dijkstra algorithm which guarantees all possible shortest path nodes are scored, and terminates early. Essentially the only occasion when passing the argument |
I think returning all simple paths is fine. I am not too concerned about optimizations for early return on this function as the number of shortest path could be exponential, so calculating all the shortest distances to all vertices is pretty reasonable |
|
I've implemented your suggestions, and at this point it feels like it's no longer a [WIP] so have updated the PR accordingly. If you'd like me to squash the commits please let me know. |
lucasvanmol
changed the title
all_shortest_paths functionall_shortest_paths function
Pull Request Test Coverage Report for Build 7378871554
💛 - Coveralls |
This (WIP) pull request aims to implement an
all_shortest_pathsalgorithm as discussed in #933.This is a draft pull request because I would like some input on how this is best implemented.
The most straightforward way to implement such a function would not require a change to
rustworkx-core; simply run Dijkstra's then look hard enough at thescoresDistanceMap returned by Dijkstra's in order to find all possible shortest paths. However, this adds some unnecessary complexity, as we could instead keep track of this information while Dijkstra's is running.In this initial commit I have included a way this could be implemented by changing the
dijkstrafunction inrustworkx-core. At this point, it does change the public rust interface, hence why I'm asking for input. Would the best way forward be a separate function? If so should this be in a separate file? Or is it not worth it, and I should just implement the approach of the previous paragraph?