On 12/7/10 9:34 AM, Minh Nguyen wrote:
Hi Jason,
On Wed, Dec 8, 2010 at 12:15 AM, Jason Grout
<[email protected]> wrote:
I worry that it is too confusing to have a min_spanning_tree function in the
documentation of the spanning_tree module that is different than the
min_spanning_tree method of a graph (different interface, more checks,
etc.).
That is nothing compared to the case where we have implemented minimum
spanning tree for digraphs in sage/graphs/digraph.py. Now when someone
wants to maintain the minimum spanning tree code, the maintainer would
be looking into at least two different modules, namely
sage/graphs/graph.py and sage/graphs/digraph.py. If an issue affects
both implementations of minimum spanning trees, i.e. those in the
directed and undirected cases, what is the chance of someone fixing
both implementations in the same patch? The idea of centralization is
that there is one place where a maintainer would go to maintain the
relevant code. Scattering code for essentially similar functionality
across more than one file is a recipe for more work on future
maintainers. NetworkX was designed as a library and many of its
implementation of graph theoretic algorithms are not written as
methods of graph classes, but rather as separate functions and
organized in separate modules. To me this makes the job of maintenance
easier. But let's move beyond that argument as I get the impression
that it has failed so far to impress anyone, just as the argument of
method bloat has failed.
How about an option 3:
* directly import the spanning_tree.min_spanning_tree function into the
graph/digraph namespaces.
That way, spanning_tree.min_spanning_tree(G) is exactly the same as
G.min_spanning_tree().
Here's an option that should preserve the current interface of
min_spanning_tree() while also allowing one to discover that method
via tab completion. Move min_spanning_tree() higher up the class
hierarchy and into sage/graphs/generic_graph.py. Once moved there,
leave the interface alone and proceed to clean-up that method as
follows:
* Handle the cases of digraphs, multigraphs, and multidigraphs.
* Handle weighted and unweighted cases of the above graph types.
I think maybe I was too brief in my suggestion to be clear. I would
favor refactoring the code out to a spanning_tree.py(x) file. My point
was that your propositions seemed to indicate that the graph class
methods that would call spanning_tree.min_spanning_tree would also do
some additional error checking, maybe have a different interface, etc.
My suggestion was to not have any additional code; put all of that error
checking, etc., into the spanning_tree.min_spanning_tree function. In
other words, the graph class would simply import the functions into the
class namespace:
class Graph:
from spanning_tree import min_spanning_tree
That way we get a standalone function in
spanning_tree.min_spanning_tree, and we also get a convenient method for
all graphs, and they are exactly the same function, have the same
interface, etc. There is no duplication of code or of doctests.
Of course, this import should probably be moved up to the generic graph
class, like you suggest, if the function can handle digraphs too.
Thanks,
Jason
--
To post to this group, send an email to [email protected]
To unsubscribe from this group, send an email to
[email protected]
For more options, visit this group at http://groups.google.com/group/sage-devel
URL: http://www.sagemath.org
