-
Notifications
You must be signed in to change notification settings - Fork 46
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
Edge weights #4: shortest path(s) #656
base: main
Are you sure you want to change the base?
Conversation
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.
Another quick partial review: it looks like there's another implementation of Floyd-Warshall in this PR, is there a reason not to use the existing implementation? Iirc the existing implementation is pretty flexible, it should be possible to use it for this too, but I might be mistaken
I've taken a look at FLOYD_WARSHALL in digraphs.c, and it would need quite a lot of reworking to use here, for 2 reasons:
Obviously we could refactor the C code, but perhaps that's a future issue? |
# The "source and destination" problem calls the "single source" problem and | ||
# extracts information for the given destination. | ||
# | ||
# Justification and benchmarks are in Raiyan's MSci thesis, Chapter 6. |
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.
Any chance of adding a link to this thesis?
local all_paths; | ||
if not source in DigraphVertices(digraph) then | ||
ErrorNoReturn("the 2nd argument <source> must be a vertex of the ", | ||
"digraph <digraph> that is the 1st argument,"); |
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.
"digraph <digraph> that is the 1st argument,"); | |
"of the 1st argument <digraph> (a digraph),"); |
or
"digraph <digraph> that is the 1st argument,"); | |
"of the 1st argument <digraph>,"); |
be a bit shorter?
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.
Assuming that the algorithms work as intended (I haven't really checked this, but I'm hoping @mtorpey and @RaiyanC did), this looks good, there are a number of minor things that should be changed, and one other thing too:
- can you please add to the doc something about the complexity of each of the functions?
We have tried to do this elsewhere in the documentation, and it's often really helpful.
function(digraph, source) | ||
if not source in DigraphVertices(digraph) then | ||
ErrorNoReturn("the 2nd argument <source> must be a vertex of the ", | ||
"digraph <digraph> that is the 1st argument,"); |
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.
Same comment as above?
local paths, v, a, current, edge_index; | ||
if not source in DigraphVertices(digraph) then | ||
ErrorNoReturn("the 2nd argument <source> must be a vertex of the ", | ||
"digraph <digraph> that is the 1st argument,"); |
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.
Same again
"digraph <digraph> that is the 1st argument,"); | ||
elif not dest in DigraphVertices(digraph) then | ||
ErrorNoReturn("the 3rd argument <dest> must be a vertex of the ", | ||
"digraph <digraph> that is the 1st argument,"); |
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.
same again
od; | ||
od; | ||
|
||
# try every triple: distance from u to v via k |
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.
Can you please add a comment about why we aren't using the C Floyd-Warshall here? Just to understand again in the future, doesn't have to be very elaborate comment.
|
||
maxNodes := nrVertices * (nrVertices - 1); | ||
|
||
# the boundary for performance is edge weight 0.125 |
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.
Can you possibly reflow this comment, so that it uses more of the width, and possibly add some punctuation if applicable? Thanks!
# detect negative cycles | ||
for i in vertices do | ||
if distances[i][i] < 0 then | ||
ErrorNoReturn("1st arg <digraph> contains a negative-weighted cycle,"); |
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.
This error message could be more helpful, possibly explain why a negative-weighted cycle is not ok, and how to compute the correct answer in this case?
if nrEdges <= threshold then | ||
return DIGRAPHS_Edge_Weighted_Johnson(digraph); | ||
else | ||
return DIGRAPHS_Edge_Weighted_FloydWarshall(digraph); |
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.
Isn't there an issue here if the graph contains a negative cycle? In particular, if it does contain a negative cycle, then shouldn't we use the other algorithm? So, wouldn't it be better to return fail
from DIGRAPHS_Edge_Weighted_FloydWarshall
in the case digraph
contains a negative cycle, and then just run DIGRAPHS_Edge_Weighted_Johnson
anyway?
to itself is considered to be 0, and so both <C>parents[<A>source</A>]</C> and | ||
<C>edges[<A>source</A>]</C> are <K>fail</K>. | ||
Edge weights can have negative values, but this operation will fail with an | ||
error if a negative-weighted cycle exists. <P/> |
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.
Not sure that this is accurate, isn't it only the case that it'll fail when the input digraph is "dense", and Floyd-Warshall is applied?
Small nitpick about the "source-destination" solution: At the moment (as documented) this calls the single-source solution and extracts the information from that; this is in many cases way less efficient than exiting early once the destination is found; This is easy to fix and shouldn't be a blocker for this PR. Just leaving this here as it will pop back up when #570 as I purposefully implemented Dijkstra with source and destination and single source for exactly that reason. |
This adds algorithms for finding shortest paths in edge-weighted digraphs, where "shortest" means "lowest total weight".
Three different "shortest path" problems are solved:
DigraphShortestPaths(digraph)
DigraphShortestPaths(digraph, source)
DigraphShortestPath (digraph, source, dest)
The "all pairs" problem has two algorithms:
The "single source" problem has three algorithms:
The "source and destination" problem calls the "single source" problem and
extracts information for the given destination.
Justification and benchmarks are in @RaiyanC's MSci thesis, Chapter 6.