-
Notifications
You must be signed in to change notification settings - Fork 61
Added DigraphMinimumCutSet
#879
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
base: main
Are you sure you want to change the base?
Conversation
…ough all triples of vertices, and instead checks center vertices of 2-edges
…2EdgeTransitive has multiple edges to doc
reiniscirpons
left a comment
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.
Thanks for the contribution, looks solid, some suggestions for docs and performance above, mostly minor.
In addition to the comments above, it looks like doc for the function is not linked in the manual, you should add
<#Include Label="DigraphMinimumCut">to z-chap5.xml in the appropriate location to get your documentation to show up in the manual (you can test this by seeing if ?DigraphMinimumCut finds the doc inside of gap).
I would also suggest changing the name of the function to DigraphMinimumCut since what its computing is the cut not the cut-set (see the definitions in https://en.wikipedia.org/wiki/Max-flow_min-cut_theorem#Cuts). It might also be good to implement a separate DigraphMinimumCutSet which wraps your current function and computes the edges.
| </ManSection> | ||
| <#/GAPDoc> | ||
|
|
||
| <#GAPDoc Label="DigraphMinimumCut"> |
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.
The name of the documentation function does not match the implementation. For what its worth I think this function should be called DigraphMinimumCut, since we are computing the cut (the set of vertices in one part of the cut) instead of the cut set (the set of edges separating both parts of the cut)
|
|
||
| <#GAPDoc Label="DigraphMinimumCut"> | ||
| <ManSection> | ||
| <Attr Name="DigraphMinimumCut" Arg="digraph, s, t"/> |
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.
Same comment about name matching doc as above
| <A>t</A>, this returns a list of two lists representing the components of | ||
| a minimal s-t cut set of <A>digraph</A>. <P/> | ||
|
|
||
| An <E>s-t cut set</E> is a partition of the vertices [S, T] such that s is in S and |
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.
| An <E>s-t cut set</E> is a partition of the vertices [S, T] such that s is in S and | |
| An <E><M>s</M>-<M>t</M> cut</E> is a partition of the vertices <M>\{S, T\}</M> such that <A>s</A> is in <M>S</M> and |
Same comment about before. Try applying markup as you would in e.g. latex to separate maths from text more. Propagate to rest of doc.
Also personal preference but I like curly braces for sets better, exercise your better judgement.
| <A>t</A>, this returns a list of two lists representing the components of | ||
| a minimal s-t cut set of <A>digraph</A>. <P/> |
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>t</A>, this returns a list of two lists representing the components of | |
| a minimal s-t cut set of <A>digraph</A>. <P/> | |
| <A>t</A>, this function returns a list of two lists representing the components of | |
| the minimum <M>s</M>-<M>t</M> cut of <A>digraph</A>. <P/> |
Changes to wording to match what the function does better, also I think we should use wrap the s-t is math markup.
| # 5. Maximum Flow and Minimum Cut | ||
| DeclareOperation("DigraphMaximumFlow", | ||
| [IsDigraph and HasEdgeWeights, IsPosInt, IsPosInt]); | ||
| DeclareOperation("DigraphMinimumCutSet", |
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.
| DeclareOperation("DigraphMinimumCutSet", | |
| DeclareOperation("DigraphMinimumCut", |
I suggest renaming the function because of the cut and cut-set distinction mentioned above.
Might be useful to implement another function DigraphMinimumCutSet which computes the set of edges separating the two parts of the partition too? Its likely just a simple wrapper around your DigraphMinimumCut function.
| return flows; | ||
| end); | ||
|
|
||
| InstallMethod(DigraphMinimumCutSet, "for an edge weighted digraph", |
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.
| InstallMethod(DigraphMinimumCutSet, "for an edge weighted digraph", | |
| InstallMethod(DigraphMinimumCut, "for an edge weighted digraph", |
Same comment as above.
| vertices := DigraphVertices(D); | ||
|
|
||
| # Check input | ||
| if not s in vertices then |
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.
Minor optimization: Because digraphs must be homogenous (all vertices are in the range [1 .. DigraphNrVertices(D)] with no holes), it might be better performance to check if s >= 1 and s <= DigraphNrVertices(D).
|
|
||
| # Find all vertices reachable from the source in this digraph | ||
| # This gives the minimal cut set by the max-flow min-cut theorem | ||
| S := Filtered(vertices, x -> IsReachable(G, s, x)); |
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.
The IsReachable function performs a BFS for each call to it, however I think you could compute all the vertices in set S by a single BFS if you implement itself. This would provide a factor of G above (since you could just check if the flow is less than the weight as you are doing the BFS).
This adds the function
DigraphMinimumCutSetusing theDigraphMaximumFlowmethod and the max-flow min-cut theorem. See issue #867.