Edge weights #4: shortest path(s) #656

mtorpey · 2024-06-07T16:27:23Z

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.

james-d-mitchell

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 · 2024-06-11T13:25:07Z

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?

james-d-mitchell · 2024-06-21T08:34:00Z

gap/weights.gi

+# 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.


Any chance of adding a link to this thesis?

james-d-mitchell · 2024-06-21T08:36:13Z

gap/weights.gi

+  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,");


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?

james-d-mitchell

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.

james-d-mitchell · 2024-06-21T08:36:37Z

gap/weights.gi

+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,");


Same comment as above?

james-d-mitchell · 2024-06-21T08:36:48Z

gap/weights.gi

+  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,");


james-d-mitchell · 2024-06-21T08:36:55Z

gap/weights.gi

+                  "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,");


james-d-mitchell · 2024-06-21T08:39:29Z

gap/weights.gi

+    od;
+  od;
+
+  # try every triple: distance from u to v via k


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.

james-d-mitchell · 2024-06-21T08:40:37Z

gap/weights.gi

+
+  maxNodes := nrVertices * (nrVertices - 1);
+
+  # the boundary for performance is edge weight 0.125


Can you possibly reflow this comment, so that it uses more of the width, and possibly add some punctuation if applicable? Thanks!

james-d-mitchell · 2024-06-21T08:41:57Z

gap/weights.gi

+  # detect negative cycles
+  for i in vertices do
+    if distances[i][i] < 0 then
+      ErrorNoReturn("1st arg <digraph> contains a negative-weighted cycle,");


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?

james-d-mitchell · 2024-06-21T08:43:47Z

gap/weights.gi

+  if nrEdges <= threshold then
+    return DIGRAPHS_Edge_Weighted_Johnson(digraph);
+  else
+    return DIGRAPHS_Edge_Weighted_FloydWarshall(digraph);


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?

james-d-mitchell · 2024-06-21T08:45:41Z

doc/weights.xml

+    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/>


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?

markuspf · 2024-08-28T10:39:31Z

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.

Add EdgeWeightedDigraphShortestPath(s)

a3f9c9d

mtorpey added the enhancement A label for PRs that provide enhancements. label Jun 7, 2024

mtorpey mentioned this pull request Jun 7, 2024

Adding Edge Weighted Algorithms #584

Open

mtorpey changed the title ~~Add edge-weighted shortest path algorithms~~ Edge weights #4: shortest path(s) Jun 7, 2024

james-d-mitchell reviewed Jun 9, 2024

View reviewed changes

mtorpey added 5 commits June 11, 2024 14:37

Fix spelling typo "adjacancy"

709d3ca

Simplify documentation

c41064a

Fix manual examples

35c662d

Linting

997d570

Add edge cases to tests

4b88960

james-d-mitchell reviewed Jun 21, 2024

View reviewed changes

james-d-mitchell requested changes Jun 21, 2024

View reviewed changes

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 mentioned this pull request Aug 28, 2024

Yen's k shortest paths iterator for edge weighted graphs #681

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edge weights #4: shortest path(s) #656

Edge weights #4: shortest path(s) #656

mtorpey commented Jun 7, 2024

james-d-mitchell left a comment

mtorpey commented Jun 11, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell left a comment

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

james-d-mitchell Jun 21, 2024

markuspf commented Aug 28, 2024

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

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


		maxNodes := nrVertices * (nrVertices - 1);

		# the boundary for performance is edge weight 0.125

Edge weights #4: shortest path(s) #656

Are you sure you want to change the base?

Edge weights #4: shortest path(s) #656

Conversation

mtorpey commented Jun 7, 2024

james-d-mitchell left a comment

Choose a reason for hiding this comment

mtorpey commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-mitchell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf commented Aug 28, 2024