#7288: Gomory-Hu Trees
----------------------------+-----------------------------------------------
Reporter: ncohen | Owner: rlm
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.3.4
Component: graph theory | Keywords:
Author: | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
----------------------------+-----------------------------------------------
Changes (by abmasse):
* status: needs_review => needs_work
Comment:
Hello, Nathann !
I looked at your patch and I'm getting more and more used to what it does.
I just added a patch that change many different small things in the
docstrings and in the code, but these are only formatting changes. I
assume you won't agree with all of them (a lot are based on my style of
programming, though I tried to respect yours as much as I could ;)) so
feel free to put anything back to what it was if you don't agree.
Before I resume the review, I have some comments/questions on your code.
1. Around line 3109 of the file generic_graph.py, you write
{{{
[p.add_constraint(v[x] + b[x][y] - v[y], min=0, max=0) for (x,y) in
g.edges(labels=None)]
}}}
Maybe I'm wrong, but it seems it would be more readable and more efficient
to simply write a `for` loop doing the same work. It is strange to create
a list that you won't use anymore in the function. There are two other
lines like this in the functions `vertex_cut` and `vertex_cover`. I
understand you do it to save a line, but is it really worth it (maybe it
is and you know why !) ?
2. The output of the function `edge_cut` and `vertex_cut` are either real
numbers or lists. Wouldn't it be better to return real numbers or tuple
instead ? There is no reason to use a list representation and tuples are
closer to the mathematical notation.
3. I changed a variable with name `l` by `label` since the letter `l`
looks very much like the number `1`.
4. In the Gomory-Hu function, you have a parameter `vertices` that you
state is not useful for the user : doesn't it mean you should remove it ?
I understand that you need this parameter for recursion purposes, but the
user shouldn't see it ! You should create a private function doing all the
work and another public one calling the private one with the initial
parameter set accordingly.
That's all for now. The rest seems fine, but I'll pay more attention to it
once you've answered. Don't forget to apply my patch on top of yours
before making any change !
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/7288#comment:11>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
