Opened 10 months ago

Closed 9 months ago

#14794 closed defect (fixed)

DiGraph constructor doc describes `boundary` option wrong

Reported by: mguaypaq Owned by: mguaypaq
Priority: major Milestone: sage-5.12
Component: graph theory Keywords: days49, graph, digraph, mutable default
Cc: ncohen Merged in: sage-5.12.beta0
Authors: Mathieu Guay-Paquet Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by 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.

Attachments (1)

trac_14794_v1.2.patch (3.1 KB) - added by mguaypaq 9 months ago.
Now with a proper commit message.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 months ago by mguaypaq

  • Description modified (diff)
  • Status changed from new to needs_review
  • Work issues fix digraph documentation, get rid of mutable defaults deleted

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

comment:2 Changed 10 months ago by mguaypaq

  • Component changed from PLEASE CHANGE to graph theory

comment:3 Changed 10 months ago by ncohen

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

comment:4 Changed 10 months ago by ncohen

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

comment:5 Changed 10 months ago by ncohen

  • Status changed from needs_review to needs_info

comment:6 Changed 10 months ago by ncohen

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

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

Nathann

comment:7 Changed 10 months ago by ncohen

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

(sorry for the ten comments)

Nathann

comment:8 follow-up: Changed 10 months ago by mguaypaq

  • Status changed from needs_info to needs_review

Replying to ncohen:

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 ncohen:

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 ncohen:

(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

comment:9 in reply to: ↑ 8 Changed 10 months ago by ncohen

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

Last edited 10 months ago by ncohen (previous) (diff)

comment:10 follow-up: Changed 10 months ago by mguaypaq

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

comment:11 in reply to: ↑ 10 Changed 10 months ago by ncohen

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

comment:12 Changed 10 months ago by ncohen

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

comment:13 Changed 10 months ago by mguaypaq

Sorry, which typo are you referring to? I don't see it anymore in the second patch (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.

comment:14 Changed 10 months ago by mguaypaq

  • Status changed from needs_review to positive_review

Travis explained the typo. Thanks everyone!

comment:15 Changed 10 months ago by mguaypaq

  • Keywords days49 added

comment:16 Changed 9 months ago by jdemeyer

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.

comment:17 Changed 9 months ago by ncohen

  • Reviewers set to Nathann Cohen

comment:18 Changed 9 months ago by jdemeyer

  • Status changed from positive_review to needs_work

Changed 9 months ago by mguaypaq

Now with a proper commit message.

comment:19 Changed 9 months ago by mguaypaq

  • Status changed from needs_work to needs_review

comment:20 Changed 9 months ago by ncohen

  • Status changed from needs_review to positive_review

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

Nathann

comment:21 Changed 9 months ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.