DiGraph constructor doc describes boundary option wrong

[mguaypaq]

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.

CC: @nathanncohen

Component: graph theory

Keywords: days49, digraph, mutable default

Author: Mathieu Guay-Paquet

Reviewer: Nathann Cohen

Merged: sage-5.12.beta0

Issue created by migration from https://trac.sagemath.org/ticket/14794

[mguaypaq]

Changed work issues from fix digraph documentation, get rid of mutable defaults to none

[mguaypaq]

comment:1

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

[nathanncohen]

comment:3

I never used this thing, and to me a code which is not documented might as well not be there, if it is only meant to be used by the guy who added it in the first place.

This being said, has it become illegal to use list as an object's attribute ? I understand that you want an immutable version of everything that is in Sage, but if it means preventing people from writing code as they want to perhaps you should change your plans O_o

By the way, who "frowns upon" mutable arguments ? It's useful to have a list somewhere, from time to time O_o

Nathann

[nathanncohen]

comment:4

The patch looks good to me, but I don't get why you simultaneously :

• Fix the docstring to say that if the input it an empty list (instead of None, formerly) then the digraph is considered to be a digraph without boundary
• Give a meaning to a None value, which is exactly what the documentation described in the first place O_o

Nathann

[nathanncohen]

comment:6

by the way there is a typo in the trac ticket's number

Get rid of mutable default argument for `boundary` (:trac:`147941)::

Nathann

[nathanncohen]

comment:7

(and you probably removed a ' by mistake at the end of a line)

(sorry for the ten comments)

Nathann

[mguaypaq]

comment:8

Replying to @nathanncohen:

I never used this thing, and to me a code which is not documented might as well not be there, if it is only meant to be used by the guy who added it in the first place.

I'm not the one who added it, but it did cause a functional problem (did you see the failing test in the ticket description?), namely the show() method blowing up.

This being said, has it become illegal to use list as an object's attribute ? I understand that you want an immutable version of everything that is in Sage, but if it means preventing people from writing code as they want to perhaps you should change your plans O_o

I know I got a previous patch rejected for using a list instead of a tuple or None as a default argument before, and I now agree with this policy (which is part of PEP 8). Why potentially shoot yourself in the foot for no benefit at all?

By the way, who "frowns upon" mutable arguments ? It's useful to have a list somewhere, from time to time O_o

The problem is only with mutable _default_ arguments. Go ahead and pass a list when you call a function whenever you want.

Replying to @nathanncohen:

The patch looks good to me, but I don't get why you simultaneously :

• Fix the docstring to say that if the input it an empty list (instead of None, formerly) then the digraph is considered to be a digraph without boundary
• Give a meaning to a None value, which is exactly what the documentation described in the first place

It's true, I'm fixing the problem in two different ways. Maybe this is overkill. Still, bringing the documentation of DiGraph in line with the documentation of Graph seemed like a good idea. Another fix would be to use () instead of None as the default argument, and then either turn it into a list later (or change the rest of the code to deal with _boundary begin a tuple, which I volunteer to do if no one objects).

I'm happy with any solution where:

• the attribute _boundary is always an iterable, and not None, since the rest of the code expects this, and
• the __init__ method does not have mutable default arguments.

If you have more/different requirements, let me know.

Replying to @nathanncohen:

(and you probably removed a ' by mistake at the end of a line)

Yes, good catch, sorry about that. The updated patch should fix this.

Cheers,
Mathieu

Apply trac_14794_v1.2.patch

[nathanncohen]

comment:9

I'm not the one who added it, but it did cause a functional problem (did you see the failing test in the ticket description?), namely the show() method blowing up.

Yeahyeah of course. I don't like this code either. Not documented, don't know what it does. Actually I would like to have a non-authoritarian way to get rid of it. Otherwise we'll jkust be shipping useless codes forever.

I know I got a previous patch rejected for using a list instead of a tuple or None as a default argument before, and I now agree with this policy (which is part of PEP 8). Why potentially shoot yourself in the foot for no benefit at all?

What exactly is the problem with default mutable arguments ? PEP8 even tells me that I should not put blank lines in my code when I think that it improves readability (cf #14562), so to me PEP8 is like the new version of my high school internal rules.

It's true, I'm fixing the problem in two different ways. Maybe this is overkill. Still, bringing the documentation of DiGraph in line with the documentation of Graph seemed like a good idea. Another fix would be to use () instead of None as the default argument, and then either turn it into a list later (or change the rest of the code to deal with _boundary begin a tuple, which I volunteer to do if no one objects).

Oh. I see. It's just that as it is, you set the default value to something that the documentation does not claim to be admissible.

I'm happy with any solution where:

• the attribute _boundary is always an iterable, and not None, since the rest of the code expects this, and
• the __init__ method does not have mutable default arguments.

If you have more/different requirements, let me know.

I don't have any requirements. Well, short of understanding why mutable default arguments are now against the Dogma O_o

Nathann

[mguaypaq]

comment:10

I looked up the old ticket where this happened to me, and it was #10304. Apparently the default argument is from http://effbot.org/zone/default-values.htm, which I've seen lots of times in various forums at this point. I just started a thread about this on sage-devel.

As for accepting None even though it's not the documented way anymore, it's just because this is a standard idiom for default values. Note that the changed documentation doesn't forbid it, just doesn't encourage it. The important point is that self._boundary is not set to None anymore.

Is this good enough for a positive review?

Cheers,
Mathieu

[nathanncohen]

comment:11

I looked up the old ticket where this happened to me, and it was #10304. Apparently the default argument is from http://effbot.org/zone/default-values.htm, which I've seen lots of times in various forums at this point. I just started a thread about this on sage-devel.

Oh, it makes sense. I thought that the default value was created at each call, but it's not the case. It's actually scary O_o

As for accepting None even though it's not the documented way anymore, it's just because this is a standard idiom for default values. Note that the changed documentation doesn't forbid it, just doesn't encourage it.

Well, you use it but don't tell others that it can be done. Anyway, it does not change much and your patch is good to go.

I just want to understand stuff before obeying it. When there is a real reason behind, it does not have to take very long :-P

Nathann

[nathanncohen]

comment:12

There is still this typo in your code with the ticket number. Could you fix it then set the ticket to positive_review ?

Thanks !

Nathann

[mguaypaq]

comment:13

Sorry, which typo are you referring to? I don't see it anymore in the second patch (attachment: trac_14794_v1.2.patch). I didn't see how to entirely replace the first patch file, so now only the second patch file should be applied.

[mguaypaq]

comment:14

Travis explained the typo. Thanks everyone!

[mguaypaq]

Changed keywords from digraph, mutable default to days49, digraph, mutable default

[jdemeyer]

comment:16

The patch needs a proper commit message (use hg qrefresh -e for that). Also, the real name of the reviewer should be filled in the Reviewer field on this ticket.

[nathanncohen]

Reviewer: Nathann Cohen

[mguaypaq]

Attachment: trac_14794_v1.2.patch.gz

Now with a proper commit message.

[nathanncohen]

comment:20

Well, if only the commit message changed... :-)

Nathann

[jdemeyer]

Merged: sage-5.12.beta0