Conversation
|
Thanks for the PR @baydrea ! Before I review this, can you please try to fix the CI? Everything has passed except linting, you can install gaplint by doing: then run it on the file and then fix the errors indicated. |
flsmith
left a comment
flsmith
left a comment
There was a problem hiding this comment.
Thanks for the pull request. I've left a few things that should be addressed before we merge this in.
| <#GAPDoc Label="UnweightedBellmanFord"> | ||
| <ManSection> | ||
| <Attr Name="UnweightedBellmanFord" Arg="digraph","source"/> | ||
| <Returns>A list of integers or <K>fail</K>.</Returns> |
There was a problem hiding this comment.
This is not what the rest of the documentation claims as the return value
| <Attr Name="UnweightedBellmanFord" Arg="digraph","source"/> | ||
| <Returns>A list of integers or <K>fail</K>.</Returns> | ||
| <Description> | ||
| If <A>digraph</A> is a digraph with <M>n</M> vertices, then this |
| <Returns>A list of integers or <K>fail</K>.</Returns> | ||
| <Description> | ||
| If <A>digraph</A> is a digraph with <M>n</M> vertices, then this | ||
| function returns a list with two sublists of <M>n</M> entries, where each entry is |
There was a problem hiding this comment.
"...a list <C>L = [distances, predecessors]</C>..." so they can be referred to later
| function returns a list with two sublists of <M>n</M> entries, where each entry is | ||
| 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 |
| 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 | ||
| the length of the shortest directed path to that i-th vertex and second sublist contains the vertex preceding that i-th |
There was a problem hiding this comment.
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."
| 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 |
There was a problem hiding this comment.
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.
| # only works for unweighted graphs, w needs to be changed into a variable | ||
| w := 1; | ||
| if distance[u] + w < distance[v] then | ||
| Print("Graph contains a negative-weight cycle"); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Can this value really ever be greater than inf?
| if distance[i] >= inf then | ||
| distance[i] := fail; | ||
| fi; | ||
| if predecessor[i] = 0 then |
There was a problem hiding this comment.
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
...
| 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 |
james-d-mitchell
added
the
gap-days-brussels-2025
wilfwilson
force-pushed
the
bellmanford
branch
from
bb363ed to
a7f5417
Compare
wilfwilson
force-pushed
the
bellmanford
branch
from
a7f5417 to
a3b4f73
Compare
wilfwilson
force-pushed
the
bellmanford
branch
from
a3b4f73 to
54e3643
Compare
3 participants
No description provided.