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

UnweightedBellmanFord #523

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions doc/attr.xml
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,37 @@ gap> DigraphShortestDistances(D);
</ManSection>
<#/GAPDoc>

<#GAPDoc Label="UnweightedBellmanFord">
<ManSection>
<Attr Name="UnweightedBellmanFord" Arg="digraph","source"/>
<Returns>A list of integers or <K>fail</K>.</Returns>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not what the rest of the documentation claims as the return value

<Description>
If <A>digraph</A> is a digraph with <M>n</M> vertices, then this
Copy link
Collaborator

Choose a reason for hiding this comment

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

and <C>source</C> is?

function returns a list with two sublists of <M>n</M> entries, where each entry is
Copy link
Collaborator

Choose a reason for hiding this comment

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

"...a list <C>L = [distances, predecessors]</C>..." so they can be referred to later

either a non-negative integer, or <K>fail</K>. <P/>

If there is a directed path from <A>source</A> to vertex <C>i</C>, then for each i-th entry the first sublist contains
Copy link
Collaborator

Choose a reason for hiding this comment

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

<C>i</C>th

the length of the shortest directed path to that i-th vertex and second sublist contains the vertex preceding that i-th
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be clearer - maybe something like "... vertex <C>i</C>, then <C>distances[i]</C> contains the length of the shortest path from <C>source</C> to vertex i, and <C>predecessors[i]</C> contains the vertex before <C>i</C> on such a shortest path."

vertex. If no such directed path exists, then the value of i is <C>fail</C>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"the value of <C>distances[i]</C> and <C>predecessors[i]</C> is <C>fail</C>.

We use the convention that the distance from every vertex to
Copy link
Collaborator

Choose a reason for hiding this comment

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

"any" rather than "every", and no need for "for all vertices..."

itself is <C>0</C> for all vertices <C>i</C>.
<P/>

<Example><![CDATA[
gap> D := Digraph([[1, 2], [3], [1, 2], [4]]);
<immutable digraph with 4 vertices, 6 edges>
gap> UnweightedBellmanFord(D, 2)
[ [ 2, 0, 1, fail ], [ 3, fail, 2, fail ] ]
gap> D := CycleDigraph(IsMutableDigraph, 3);
<mutable digraph with 3 vertices, 3 edges>
gap> UnweightedBellmanFord(D, 3);
[ [ 1, 2, 0 ], [ 3, 1, fail ] ]
]]></Example>
</Description>
</ManSection>
<#/GAPDoc>


<#GAPDoc Label="DigraphDiameter">
<ManSection>
<Attr Name="DigraphDiameter" Arg="digraph"/>
Expand Down
1 change: 1 addition & 0 deletions gap/attr.gd
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ DeclareAttribute("DigraphDegeneracy", IsDigraph);
DeclareAttribute("DigraphDegeneracyOrdering", IsDigraph);
DeclareAttribute("DIGRAPHS_Degeneracy", IsDigraph);
DeclareAttribute("DigraphShortestDistances", IsDigraph);
DeclareOperation("UnweightedBellmanFord", [IsDigraph, IsPosInt]);
DeclareAttribute("DigraphDiameter", IsDigraph);
DeclareAttribute("DigraphGirth", IsDigraph);
DeclareAttribute("DigraphOddGirth", IsDigraph);
Expand Down
46 changes: 46 additions & 0 deletions gap/attr.gi
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,52 @@ end);
# returns the vertices (i.e. numbers) of <D> ordered so that there are no
# edges from <out[j]> to <out[i]> for all <i> greater than <j>.

InstallMethod(UnweightedBellmanFord, "for a digraph by out-neighbours",
[IsDigraph, IsPosInt],
function(digraph, source)
local distance, n, predecessor, i, inf, u, v, edge, w;
n := DigraphNrVertices(digraph);
# wouldn't work for weighted digraphs
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment isn't necessary - the name of the function is enough.

inf := n + 1;
distance := List([1 .. n], x -> 0);
predecessor := List([1 .. n], x -> 0);
for i in DigraphVertices(digraph) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

This loop is unnecessary - you should just do distance := List([1 .. n], x -> inf) to initialise the distances list to contain the value inf, and the same for predecessor (which already contains all 0s from the line above).

Better would be to use the descriptive function ListWithIdenticalEntries(n, inf).

distance[i] := inf;
predecessor[i] := 0;
od;
distance[source] := 0;
for i in [1 .. n - 1] do
for edge in DigraphEdges(digraph) do
u := edge[1];
v := edge[2];
# only works for unweighted graphs, w needs to be changed into a variable
Copy link
Collaborator

Choose a reason for hiding this comment

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

But for this function which is only supposed to do the unweighted version, is there any reason to have this comment or use the variable w? Particularly since w is not very descriptive.
There is some sort of argument for using a constant edge_weight := 1 defined near the start of the function, but most people would probably just replace w by 1 everywhere.

w := 1;
if distance[u] + w < distance[v] then
distance[v] := distance[u] + w;
predecessor[v] := u;
fi;
od;
od;
for edge in DigraphEdges(digraph) do
u := edge[1];
v := edge[2];
# only works for unweighted graphs, w needs to be changed into a variable
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above.

w := 1;
if distance[u] + w < distance[v] then
Print("Graph contains a negative-weight cycle");
Copy link
Collaborator

Choose a reason for hiding this comment

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

We almost never want our functions to Print things. In the unweighted case, there's no need for this check is there? It's impossible to have a negative-weight cycle when all edges have weight 1.

fi;
od;
for i in DigraphVertices(digraph) do
if distance[i] >= inf then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this value really ever be greater than inf?

distance[i] := fail;
fi;
if predecessor[i] = 0 then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be tempted to use inf to represent failure for both distance and predecessor, rather than inf and 0 - especially since they can only fail at the same time. Since they can only fail at the same time, you could also replace the two loops with one that looks like

for i in DigraphVertices(digraph) do
    if distance[i] = inf or predecessor[i] = inf then
        ...

predecessor[i] := fail;
fi;
od;
return [distance, predecessor];
end);

InstallMethod(DigraphTopologicalSort, "for a digraph by out-neighbours",
[IsDigraphByOutNeighboursRep],
D -> DIGRAPH_TOPO_SORT(OutNeighbours(D)));
Expand Down
18 changes: 18 additions & 0 deletions tst/standard/attr.tst
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,24 @@ gap> DIGRAPH_ConnectivityDataForVertex(gr, 2);;
gap> DigraphShortestDistances(gr);
[ [ 0, 1, 1 ], [ 1, 0, 1 ], [ 1, 1, 0 ] ]

# UnweightedBellmanFord
gap> gr := Digraph([[1, 2], [3], [1, 2], [4]]);
<immutable digraph with 4 vertices, 6 edges>
gap> UnweightedBellmanFord(gr, 2);
[ [ 2, 0, 1, fail ], [ 3, fail, 2, fail ] ]
gap> gr := CycleDigraph(IsMutableDigraph, 3);
<mutable digraph with 3 vertices, 3 edges>
gap> UnweightedBellmanFord(gr, 3);
[ [ 1, 2, 0 ], [ 3, 1, fail ] ]
gap> gr := Digraph([[], []]);
<immutable empty digraph with 2 vertices>
gap> UnweightedBellmanFord(gr, 2);
[ [ fail, 0 ], [ fail, fail ] ]
gap> gr := Digraph([[1], [2], [3], [4]]);
<immutable digraph with 4 vertices, 4 edges>
gap> UnweightedBellmanFord(gr, 2);
[ [ fail, 0, fail, fail ], [ fail, fail, fail, fail ] ]

# OutNeighbours and InNeighbours
gap> gr := Digraph(rec(DigraphNrVertices := 10,
> DigraphSource := [1, 1, 5, 5, 7, 10],
Expand Down