-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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?
There was a problem hiding this 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.
@james-d-mitchell Did @ffloresbrito address your changes? Is @ffloresbrito still involved in any GAP-related activity? |
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. |
7671666
to
f26545c
Compare
5b55e83
to
7aa5e9d
Compare
There was a problem hiding this 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.
Looks like all of the specific changes that James asked for were made.
There was a problem hiding this 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.
7aa5e9d
to
7e1f0d7
Compare
7e1f0d7
to
1f0981b
Compare
e21ba66
to
da6864e
Compare
da6864e
to
059d12b
Compare
Stale pull request message |
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.