#14794: DiGraph constructor doc describes `boundary` option wrong
---------------------------------------------------+------------------------
Reporter: mguaypaq | Owner: mguaypaq
Type: defect | Status:
needs_review
Priority: major | Milestone: sage-5.12
Component: PLEASE CHANGE | Resolution:
Keywords: graph, digraph, mutable default | Work issues:
Report Upstream: N/A | Reviewers:
Authors: Mathieu Guay-Paquet | Merged in:
Dependencies: | Stopgaps:
---------------------------------------------------+------------------------
Changes (by mguaypaq):
* status: new => needs_review
* work_issues: fix digraph documentation, get rid of mutable defaults =>
Old description:
> While working on quivers `sage.quivers.*` we ran into some confusing
> documentation in `DiGraph` about the `boundary` argument of the
> constructor:
>
> {{{
> - ``boundary`` - a list of boundary vertices, if none,
> digraph is considered as a 'digraph without boundary'
> }}}
>
> Here, it turns out that `None` is not actually allowed, and instead an
> empty list `[]` should be used instead. This is described more clearly in
> the `Graph` docstring:
>
> {{{
> - ``boundary`` - a list of boundary vertices, if
> empty, graph is considered as a 'graph without boundary'
> }}}
>
> An example of a problem:
>
> {{{
> sage: DiGraph()
> Digraph on 0 vertices
> sage: show(DiGraph())
> sage: show(DiGraph(boundary=[]))
> sage: show(DiGraph(boundary=None))
> ---------------------------------------------------------------------------
> TypeError Traceback (most recent call
> last)
> ...
> TypeError: object of type 'NoneType' has no len()
> }}}
>
> As a separate point, both `Graph.__init__` and `DiGraph.__init__` have
> mutable default arguments (`def __init__(..., boundary=[], ...)`) which
> is frowned upon.
>
> I'll put up a patch in a few minutes.
New description:
While working on quivers `sage.quivers.*` we ran into some confusing
documentation in `DiGraph` about the `boundary` argument of the
constructor:
{{{
- ``boundary`` - a list of boundary vertices, if none,
digraph is considered as a 'digraph without boundary'
}}}
Here, it turns out that `None` is not actually allowed, and instead an
empty list `[]` should be used instead. This is described more clearly in
the `Graph` docstring:
{{{
- ``boundary`` - a list of boundary vertices, if
empty, graph is considered as a 'graph without boundary'
}}}
An example of a problem:
{{{
sage: DiGraph()
Digraph on 0 vertices
sage: show(DiGraph())
sage: show(DiGraph(boundary=[]))
sage: show(DiGraph(boundary=None))
---------------------------------------------------------------------------
TypeError Traceback (most recent call
last)
...
TypeError: object of type 'NoneType' has no len()
}}}
As a separate point, both `Graph.__init__` and `DiGraph.__init__` have
mutable default arguments (`def __init__(..., boundary=[], ...)`) which is
frowned upon.
--
Comment:
The patch is now up.
The `_boundary` attribute seems to be only used in the files:
{{{
graphs/graph_plot.py: GraphPlot.set_vertices assumes _boundary is iterable
graphs/generic_graph.py: Lots of code assumes (or enforces) that _boundary
is a list
graphs/graph.py: only used in __init__
graphs/digraph.py: only used in __init__, wrong documentation
}}}
None of the code modifies the list, and in fact some of it goes to some
pains to make sure appropriate copies are made. Would there be any
objections to replacing the list by a tuple everywhere?
Apply trac_14794_v1.patch
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14794#comment:1>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/groups/opt_out.
