#13698: Access to graph routines of the GLPK
------------------------------------------------------------------------+---
Reporter: christiankuper |
Owner: jason, jkantor
Type: enhancement |
Status: needs_work
Priority: major |
Milestone: sage-5.5
Component: numerical |
Resolution:
Keywords: out-of-kilter, minflow, maxflow, critical path, GLPK |
Work issues:
Report Upstream: N/A |
Reviewers:
Authors: Christian Kuper |
Merged in:
Dependencies: |
Stopgaps:
------------------------------------------------------------------------+---
Changes (by ncohen):
* status: needs_review => needs_work
Comment:
Hellooooooooo Christian !!!
Your patch is still waiting for a review, but such large patches eat my
week-ends. And I also have #13808 too deal with `T_T`
I uploaded a patch which does some (uninteresting) changes to your patch.
I also have a list of comments. I can see that you put a lot of work in
this patch, but you add a big amount of things in Sage so it is bound to
take some time again `^^;`
* `create_graph method` : why is this a method, and not just part of the
__init__ function ? If I do the following, it looks like memory keeps
being allocated and is never freed !
{{{
gbe = GLPKGraphBackend()
gbe.create_graph()
gbe.create_graph()
gbe.create_graph()
...
}}}
* Can't all occurrences of `if self.graph is NULL:` be removed from the
file ? self.graph() can never be null !
* self.graph() can be null if something went wrong with the memory
allocation. An exception should be raised if that happens.
* I had never seen this `property` Python thing but it feels weird to have
two ways to set/get this graph_name parameter. Could you chose between one
of those ? (same for other properties defined in the module)
* Note from personal experience : I regret to have exposed "name"
functions in the LP interface. It has always been a mess to maintain, it
takes a loooooooooot of code just to deal with it, and I really do not see
the point. It's up to you in this case but in you stead I would not expose
anything related to names `^^;`
* It looks like you need to free memory in methods like `del_vertices`
when you allocate it, for instance your `num` variable.
* Considering that the Graph methods in Sage are `Graph.add_vertices` and
`Graph.delete_vertices`, could you copy the same names in your interface ?
Or do you think that it is a bad idea ? (same with edges)
* `arc_exists` is `has_edge` in Sage. Or `has_arc` if you prefer.
* I had not seen this `delete_graph` method when I made my first comment.
But here again, I feel that this should not be a meethod, and be a part of
`__destruct__` instead `:-)`
* `read_graph` : in Sage's way of doing things, this should be part of the
constructor. Something like "you can build an empty graph, or build it
from a file`
* It would be nice to have an index of methods at the top of the module's
documentation, as we have at the top of the following page :
http://www.sagemath.org/doc/reference/sage/graphs/graph.html. It takes
some time to do it, and it is hell to do unless you know how to use
emacs/vim macros.. If you feel like doing it, I will do it myself when
this patch will be almost done.
* It would be nice to say what ccdata is supposed to be, somewhere.
* It would make sense to update GenericGraph.flow() so that it can use
GLPK as a solver. Actually, the best would be to have in your new module a
function converting a Sage graph to a GLPK graph, so that the code in
GenericGraph would stay short. This way we could also begin to compare
performances.
Have a nice sunday eveniiiiiiiiiiiiiing ! `:-P`
Nathann
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13698#comment:3>
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.
