#6679: Vertex Coloring, Edge Coloring
----------------------------+-----------------------------------------------
Reporter: ncohen | Owner: rlm
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.3
Component: graph theory | Keywords:
Work_issues: | Author:
Upstream: N/A | Reviewer:
Merged: |
----------------------------+-----------------------------------------------
Changes (by mvngu):
* status: needs_review => needs_work
* upstream: => N/A
Comment:
I assume that the patch `trac_6679.patch` is the one to review, so here
goes. Here are some issues:
1. For the function `chromatic_number()`, it's a bad idea to make "MILP"
the default algorithm choice because that algorithm requires the
installation of an optional package. Imagine the surprise (and confusion)
when one uses the function `chromatic_number()` with the default value,
but neither GLPK nor CBC is installed. You can make some algorithm a
default choice provided that it's in a standard package or in the Sage
library. If you have another algorithm that uses an optional package, you
could still provide an interface to that algorithm and document uses of
that algorithm in the docstring. So between "CP" and "DLX", choose one and
make that algorithm the default choice. You have clearly documented how to
use the "MILP" algorithm, which is good for those who want a better
algorithm.
1. You don't need to put spaces between the sign "=" if it occurs as part
of assigning a value to an argument in a function call, like in
`chromatic_number(algorithm="MILP")`. To follow coding conventions, you
should put spaces between operators. So instead of doing
`algorithm=="CP"`, you should do `algorithm == "CP"`. You can read more
about Python coding conventions in
[http://www.python.org/dev/peps/pep-0008/ PEP 8].
1. Use the style of raising exceptions that is compatible with Python
3.x. For example, the following is not recommended for being compatible
with Python 3.x:
{{{
raise ValueError, "The `algorithm` variable must be set to either `DLX`,
`MILP` or `CP`."
}}}
However, the following style is compatible with Python 3.x:
{{{
raise ValueError("The `algorithm` variable must be set to either `DLX`,
`MILP` or `CP`.")
}}}
Lots of code in the Sage library still uses the old style of raising
exceptions. This will come back to haunt us when the time comes to switch
to Python 3.x.
1. The formatting of docstring is inconsistent.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6679#comment:13>
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.
