Add VertexConnectivity
#94
Conversation
There was a problem hiding this comment.
A few more fairly minor comments. I've still not studied the algorithm in detail, to see whether you're implementing it correctly. Not sure whether I will. Maybe @james-d-mitchell knows more about the algorithm and could weigh in?
james-d-mitchell
added
enhancement
|
@james-d-mitchell Did @ffloresbrito address your changes? Is @ffloresbrito still involved in any GAP-related activity? |
wilfwilson
force-pushed
the
vertex-connectivity
branch
from
de81d16
to
b27af13
Compare
|
@james-d-mitchell I have squashed and rebased this PR on the latest master branch, and fixed the CI errors, and pushed. Assuming that @ffloresbrito is no longer involved with this, we can now decide how to proceed with this PR (or whether just to just close it). On the whole, I'd say it's better to have a slow implementation than no implementation - it could always be a student project to improve or rewrite it. But that assumes that this implementation is correct, and it's been long enough that I don't remember if it is. So I'll review it again sometime. |
wilfwilson
force-pushed
the
vertex-connectivity
branch
2 times, most recently
from
7671666
to
f26545c
Compare
wilfwilson
force-pushed
the
vertex-connectivity
branch
2 times, most recently
from
5b55e83
to
7aa5e9d
Compare
wilfwilson
dismissed
james-d-mitchell’s stale review
Looks like all of the specific changes that James asked for were made.
There was a problem hiding this comment.
I've gone through the specific changes that James and I requested, and everything (bar the comment above) was resolved. So I've marked the comments as such.
I think we can assume that @ffloresbrito is no longer involved with this PR. I'm going to give it a proper review, and if it requires only small changes to make it acceptable, then I'll do this and get the PR merged.
wilfwilson
force-pushed
the
vertex-connectivity
branch
from
7aa5e9d
to
7e1f0d7
Compare
wilfwilson
force-pushed
the
vertex-connectivity
branch
from
7e1f0d7
to
1f0981b
Compare
wilfwilson
force-pushed
the
vertex-connectivity
branch
3 times, most recently
from
e21ba66
to
da6864e
Compare
wilfwilson
force-pushed
the
vertex-connectivity
branch
from
da6864e
to
059d12b
Compare
There was a problem hiding this comment.
For us this method would be very helpful. I looked at the algorithm and the implementation and to me it looks like a useful implementation of vertex connectivity.
In my opinion two parts are written a bit awkwardly as you can see below. Is there anything else that needs to be done apart from adding a citation to the documentation?
| mindeg := degs[i]; | ||
| mindegv := i; | ||
| fi; | ||
| od; |
There was a problem hiding this comment.
I think mindegv:=PositionMinimum(degs) and mindeg:=degs[mindegv] should do the same and is an easier computation and easier to read.
| degs[j] := degs[j] + 1; | ||
| fi; | ||
| od; | ||
| od; |
There was a problem hiding this comment.
It should be also possible to compute degs with OutDegrees and InDegrees such that it is maybe shorter and easier to read.
james-d-mitchell
removed
the
do not merge
reiniscirpons
force-pushed
the
vertex-connectivity
branch
from
059d12b
to
29a624d
Compare
Edit: ( @reiniscirpons ): the current implementation seems have some bug and it fails on
DigraphFromGraph6String("HoStIv{")(this is Graph 33668 on house of graphs). The expected output is 4, currently the method gives 3. My hunch is that the bug may be in the Edmonds-Karp method. It seems PR #584 implements a max-flow method, so once this is merged, we may be able to fix this PR as well.Hopefully within the next 7 years we will be able to merge this PR!
Original comment:
This pull request adds an attribute called
VertexConnectivitywhich for a graph G = (V, E) computes the least cardinality |S| of a subset S of V, such that G - S is either disconnected, is trivial, or has no vertices. Documentation and tests are included.