UnweightedBellmanFord #523
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. |
doc/attr.xml
Outdated
|
|
||
| If there is a directed path from <A>source</A> to vertex <C>i</C>, then | ||
| the value of the entry i in the first sublist is the length of the shortest such directed | ||
| path and the entry i in the second sublist is the vertex preceding the vertex of that entry. If no such directed path exists, then the value of i is <C>fail</C>. We use the convention that the distance from every vertex to |
There was a problem hiding this comment.
I'm not sure what the vertex preceding the vertex of that entry means, can you please clarify? And also please wrap the line so that it is at most 80 chars long.
| <#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 |
|
Stale pull request message |
No description provided.