Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mtorpey
Copy link
Collaborator

@mtorpey mtorpey commented Jun 7, 2024

This adds algorithms for finding shortest paths in edge-weighted digraphs, where "shortest" means "lowest total weight".

Three different "shortest path" problems are solved:

  • All pairs: DigraphShortestPaths(digraph)
  • Single source: DigraphShortestPaths(digraph, source)
  • Source and destination: DigraphShortestPath (digraph, source, dest)

The "all pairs" problem has two algorithms:

  • Johnson: better for sparse digraphs
  • Floyd–Warshall: better for dense graphs

The "single source" problem has three algorithms:

  • If "all pairs" is already known, extract information for the given source
  • Dijkstra: faster, but cannot handle negative weights
  • Bellman–Ford: slower, but handles negative weights

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.

@mtorpey mtorpey added the enhancement A label for PRs that provide enhancements. label Jun 7, 2024
@mtorpey mtorpey changed the title Add edge-weighted shortest path algorithms Edge weights #4: shortest path(s) Jun 7, 2024
Copy link
Member

@james-d-mitchell james-d-mitchell left a 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

@mtorpey
Copy link
Collaborator Author

mtorpey commented Jun 11, 2024

Is there a reason not to use the existing implementation [of Floyd–Warshall]? Iirc the existing implementation is pretty flexible.

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:

  • It always creates a distance matrix consisting of two values (edge and non-edge) rather than multiple values
  • It only stores distances, and doesn't remember info about the actual paths.

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.
Copy link
Member

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,");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"digraph <digraph> that is the 1st argument,");
"of the 1st argument <digraph> (a digraph),");

or

Suggested change
"digraph <digraph> that is the 1st argument,");
"of the 1st argument <digraph>,");

be a bit shorter?

Copy link
Member

@james-d-mitchell james-d-mitchell left a 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,");
Copy link
Member

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,");
Copy link
Member

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,");
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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,");
Copy link
Member

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);
Copy link
Member

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/>
Copy link
Member

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?

@james-d-mitchell james-d-mitchell added the waiting for creator input A label for issues/PRs where we are waiting for the creator to do something label Jun 21, 2024
@markuspf
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A label for PRs that provide enhancements. waiting for creator input A label for issues/PRs where we are waiting for the creator to do something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants