Add DigraphShortestPath method #148

MTWhyte · 2018-11-21T16:55:17Z

This pull request implements a DigraphShortestPath method, which takes a digraphs and two positive integers, and returns a list in the same format as DigraphPath.

The shortest path is found using a breadth-first search.

james-d-mitchell

Thanks @MTWhyte this looks really good for a first PR!! I've made a number of suggestions, please revise the PR and we can then move forward with it.

james-d-mitchell · 2018-11-23T09:41:41Z

doc/oper.xml

+  <Returns>A pair of lists, or <K>fail</K>.</Returns>
+  <Description>
+    If there exists a non-trivial directed path (or a non-trivial cycle, in the
+    case that <A>u</A> <C>=</C> <A>v</A>) from vertex <A>u</A> to vertex


This is a minor quibble, but I think it'd be better if there were no parentheses in this sentence. Better possibly as two sentences:

Returns the shortest directed path in the digraph digraph from the vertex u to the vertex v, if such a path exists. If u = v, then the shortest cycle of length greater than 1 is returned, again, if it exists.

In reply to @james-d-mitchell: this should be either shortest cycle of length greater than 0, or shortest non-trivial cycle, as discussed below.

james-d-mitchell · 2018-11-23T09:44:17Z

gap/oper.gi

+  distance := [];
+
+  if u = v and v in nbs[v] then  # Considers the trivial path from v to v
+    return [[v, v], [Position(nbs[v], v)]];


Hmm, does this agree with the documentation? Isn't this condition true if u = v and there is a loop at u? Or, is it that the loop has to be of length > 0, not > 1 as I somehow assumed before??

In reply to @james-d-mitchell: the definition of trivial in the package documentation:

The length of a directed walk (v1,e1,v2,e2,...,en−1,vn) is equal to n−1, the number of edges it contains. A directed walk of zero length, i.e. a sequence (v) for some vertex v, is called trivial. A trivial directed walk is considered to be both a circuit and a cycle, as is the empty directed walk ().

By the original documentation in this PR:

(or a non-trivial cycle, in the case that <A>u</A> <C>=</C> <A>v</A>)

a loop therefore qualifies as the shortest path, since a loop is a non-trivial cycle.

However, I therefore suggest adjusting the misleading comment in the line above.

james-d-mitchell · 2018-11-23T09:45:57Z

gap/oper.gi

+  #  Setting up objects useful in the function.
+  for i in verts do
+    Add(distance, -1);
+  od;


You could replace this entire for-loop with: distance := ListWithIdenticalEntries(Length(verts), -1);

james-d-mitchell · 2018-11-23T09:46:58Z

gap/oper.gi

+  for i in verts do
+    Add(next, false);
+    Add(falselist, false);
+  od;


Similar, to the previous comment, this could be next := BlistList([1 .. Length(verts)], []);
and falselist := BlistList([1 .. Length(verts)], []);

james-d-mitchell · 2018-11-23T09:49:09Z

gap/oper.gi

+    Add(falselist, false);
+  od;
+
+  n := 0;  # Counts the loops


I suggest either removing or improving the comment here. "The loops" could be misconstrued as meaning loops in the digraph, which is not what you mean I think.

james-d-mitchell · 2018-11-23T09:52:27Z

gap/oper.gi

+            # Finds the path
+            for i in [1 .. n] do
+              Add(path[1], b);
+              Add(path[2], Position(nbs[parent[b]], b));


The Position(nbs[parent[b]], b) lookup is potentially time consuming (possible linear search in nbs[parent[b]]). You could avoid this altogether by having another list edge, replacing the for-loop for b in nbs[a] do by for j in [1 .. Length(nbs[a])] do b := nbs[a][i]; and after parent[b] := a writing edge[b] := i, and the replacing Position(nbs[parent[b]], b) by Add(path[2], edge[b]). I hope that this makes sense.

james-d-mitchell · 2018-11-23T09:57:46Z

gap/oper.gi

+InstallMethod(DigraphShortestPath, "for a digraph and two pos ints",
+[IsDigraph, IsPosInt, IsPosInt],
+function(digraph, u, v)
+  local current, next, parent, distance, falselist, verts, nbs, n, a, b, i, path;


You should add some basic checks at the start of this function too. For example, the start of DigraphPath is what's written below. I think you should probably have the same thing at the start of this method.

verts := DigraphVertices(digraph); if not (u in verts and v in verts) then ErrorNoReturn("Digraphs: DigraphPath: usage,\n", "the second and third arguments <u> and <v> must be\n", "vertices of the first argument <digraph>,"); fi; if IsDigraphEdge(digraph, [u, v]) then return [[u, v], [Position(OutNeighboursOfVertex(digraph, u), v)]]; elif HasIsTransitiveDigraph(digraph) and IsTransitiveDigraph(digraph) then # If it's a known transitive digraph, just check whether the edge exists return fail; # Glean information from WCC if we have it elif HasDigraphConnectedComponents(digraph) and DigraphConnectedComponents(digraph).id[u] <> DigraphConnectedComponents(digraph).id[v] then return fail; fi;

I've added these tests to the start of the function. I believe the

if IsDigraphEdge(digraph, [u, v]) then return [[u, v], [Position(OutNeighboursOfVertex(digraph, u), v)]];

does much the same thing as the existing

if u = v and v in nbs[v] then # Considers the trivial path from v to v return [[v, v], [Position(nbs[v], v)]];

which was the subject of a previous comment. I shall just remove the latter for simplicity.

james-d-mitchell · 2018-11-23T09:58:06Z

gap/oper.gi

+            Add(path[1], u);  # Adds the starting vertex to the list of vertices.
+            return [Reversed(path[1]), Reversed(path[2])];
+          fi;
+


Please remove this blank line.

james-d-mitchell · 2018-11-23T09:58:30Z

gap/oper.gi

+
+        od;
+      od;
+


Please remove the two blank lines here too.

james-d-mitchell · 2018-11-23T10:00:01Z

gap/oper.gi

+      od;
+
+      current := ListBlist(verts, next);
+      next    := IntersectionBlist(next, falselist);


I think (not 100%) that you'd be better off doing IntersectBlist(next, falselist);. The reason being that IntersectionBlist produces a new list, whereas IntersectBlist changes next in-place.

james-d-mitchell · 2018-11-23T10:01:46Z

gap/oper.gi

+
+  n := 0;  # Counts the loops
+  while current <> [] do
+    n    := n + 1;


Weird spacing before the := here.

james-d-mitchell · 2018-11-23T10:02:02Z

gap/oper.gi

+      next    := IntersectionBlist(next, falselist);
+
+    od;
+      return fail;


Bad indentation.

wilfwilson

Thank you for this! I've added some comments in relation to James's ones.

wilfwilson · 2018-11-23T10:42:07Z

doc/oper.xml

+  <Returns>A pair of lists, or <K>fail</K>.</Returns>
+  <Description>
+    If there exists a non-trivial directed path (or a non-trivial cycle, in the
+    case that <A>u</A> <C>=</C> <A>v</A>) from vertex <A>u</A> to vertex


In reply to @james-d-mitchell: this should be either shortest cycle of length greater than 0, or shortest non-trivial cycle, as discussed below.

wilfwilson · 2018-11-23T10:43:32Z

gap/oper.gi

+  distance := [];
+
+  if u = v and v in nbs[v] then  # Considers the trivial path from v to v
+    return [[v, v], [Position(nbs[v], v)]];


In reply to @james-d-mitchell: the definition of trivial in the package documentation:

The length of a directed walk (v1,e1,v2,e2,...,en−1,vn) is equal to n−1, the number of edges it contains. A directed walk of zero length, i.e. a sequence (v) for some vertex v, is called trivial. A trivial directed walk is considered to be both a circuit and a cycle, as is the empty directed walk ().

By the original documentation in this PR:

(or a non-trivial cycle, in the case that <A>u</A> <C>=</C> <A>v</A>)

a loop therefore qualifies as the shortest path, since a loop is a non-trivial cycle.

However, I therefore suggest adjusting the misleading comment in the line above.

wilfwilson · 2018-11-23T10:44:49Z

gap/oper.gi

+  nbs      := OutNeighbors(digraph);
+  distance := [];
+
+  if u = v and v in nbs[v] then  # Considers the trivial path from v to v


A loop is not a trivial path. Therefore please change this comment, to something like # is u = v, and there is a loop at v?

james-d-mitchell · 2019-01-30T14:52:13Z

I'm happy for this to be merged, but it needs to be rebased on the current master first.

james-d-mitchell added 0.14 new-feature A label for new features. labels Nov 23, 2018

james-d-mitchell requested changes Nov 23, 2018

View reviewed changes

james-d-mitchell reviewed Nov 23, 2018

View reviewed changes

gap/oper.gi Outdated

n := 0; # Counts the loops

while current <> [] do

n := n + 1;

Copy link

Member

james-d-mitchell Nov 23, 2018

Choose a reason for hiding this comment

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

Weird spacing before the := here.

james-d-mitchell reviewed Nov 23, 2018

View reviewed changes

gap/oper.gi Outdated

next := IntersectionBlist(next, falselist);

od;

return fail;

Copy link

Member

james-d-mitchell Nov 23, 2018

Choose a reason for hiding this comment

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

Bad indentation.

wilfwilson reviewed Nov 23, 2018

View reviewed changes

wilfwilson added 0.15 and removed 0.14 labels Nov 23, 2018

james-d-mitchell approved these changes Jan 30, 2019

View reviewed changes

wilfwilson approved these changes Jan 30, 2019

View reviewed changes

Add ShortestPath method

e712e7e

MTWhyte force-pushed the shortest-path-2 branch from 06ed6ef to e712e7e Compare January 30, 2019 15:19

james-d-mitchell merged commit 4902698 into digraphs:master Jan 31, 2019

MTWhyte deleted the shortest-path-2 branch June 21, 2019 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DigraphShortestPath method #148

Add DigraphShortestPath method #148

MTWhyte commented Nov 21, 2018

james-d-mitchell left a comment

james-d-mitchell Nov 23, 2018

wilfwilson Nov 23, 2018 •

edited

Loading

james-d-mitchell Nov 23, 2018

wilfwilson Nov 23, 2018 •

edited

Loading

james-d-mitchell Nov 23, 2018

james-d-mitchell Nov 23, 2018

james-d-mitchell Nov 23, 2018

james-d-mitchell Nov 23, 2018

james-d-mitchell Nov 23, 2018

MTWhyte Nov 28, 2018

james-d-mitchell Nov 23, 2018

james-d-mitchell Nov 23, 2018

james-d-mitchell Nov 23, 2018 •

edited by wilfwilson

Loading

james-d-mitchell Nov 23, 2018

james-d-mitchell Nov 23, 2018

wilfwilson left a comment

wilfwilson Nov 23, 2018 •

edited

Loading

wilfwilson Nov 23, 2018 •

edited

Loading

wilfwilson Nov 23, 2018

james-d-mitchell commented Jan 30, 2019

Add DigraphShortestPath method #148

Add DigraphShortestPath method #148

Conversation

MTWhyte commented Nov 21, 2018

james-d-mitchell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilfwilson Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilfwilson Nov 23, 2018 • edited Loading

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

james-d-mitchell Nov 23, 2018 • edited by wilfwilson Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilfwilson left a comment

Choose a reason for hiding this comment

wilfwilson Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

wilfwilson Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-mitchell commented Jan 30, 2019

wilfwilson Nov 23, 2018 •

edited

Loading

wilfwilson Nov 23, 2018 •

edited

Loading

james-d-mitchell Nov 23, 2018 •

edited by wilfwilson

Loading

wilfwilson Nov 23, 2018 •

edited

Loading

wilfwilson Nov 23, 2018 •

edited

Loading