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

Add VertexConnectivity #94

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ffloresbrito
Copy link

@ffloresbrito ffloresbrito commented Nov 1, 2017

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 VertexConnectivity which 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.

doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

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?

doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
wilfwilson
wilfwilson previously approved these changes Nov 1, 2017
Copy link
Member

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

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

This looks quite reasonable, apart from some design decisions that are likely rather costly (in terms of time and space). I'd like to see the code in this PR refactored to remove these things.

doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
@james-d-mitchell james-d-mitchell added enhancement A label for PRs that provide enhancements. do not merge A label for PRs that should not be merged for whatever reason. 0.12 and removed 0.11 labels Nov 21, 2017
@wilfwilson wilfwilson added 0.13 and removed 0.12 labels Jan 31, 2018
@wilfwilson wilfwilson added 0.14 and removed 0.13 labels Sep 18, 2018
@wilfwilson wilfwilson added 0.15 and removed 0.14 labels Nov 23, 2018
@wilfwilson
Copy link
Collaborator

@james-d-mitchell Did @ffloresbrito address your changes? Is @ffloresbrito still involved in any GAP-related activity?

@wilfwilson wilfwilson removed the 0.15 label Aug 22, 2019
@wilfwilson wilfwilson changed the title add VertexConnectivity Add VertexConnectivity Mar 3, 2021
@wilfwilson
Copy link
Collaborator

@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 wilfwilson force-pushed the vertex-connectivity branch 2 times, most recently from 7671666 to f26545c Compare March 3, 2021 18:48
@digraphs digraphs deleted a comment from codecov bot Mar 3, 2021
@wilfwilson wilfwilson force-pushed the vertex-connectivity branch 2 times, most recently from 5b55e83 to 7aa5e9d Compare March 10, 2021 07:53
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

I accidentally marked a comment from James as resolved, but it's not. It basically said:

You should add a citation to the documentation, stating where this algorithm is taken from.

@wilfwilson wilfwilson dismissed james-d-mitchell’s stale review March 10, 2021 13:10

Looks like all of the specific changes that James asked for were made.

@wilfwilson wilfwilson self-assigned this Mar 10, 2021
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

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 wilfwilson changed the title Add VertexConnectivity Add VertexConnectivity Mar 26, 2021
@wilfwilson wilfwilson force-pushed the vertex-connectivity branch 3 times, most recently from e21ba66 to da6864e Compare May 26, 2021 19:35
@digraphs digraphs deleted a comment from github-actions bot Aug 26, 2024
Copy link
Contributor

@MeikeWeiss MeikeWeiss left a comment

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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 james-d-mitchell removed the do not merge A label for PRs that should not be merged for whatever reason. label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A label for PRs that provide enhancements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants