add_vertex w/o params should return the new vertex

[ddestrada]

It's nice to have a way to add a new vertex no matter what the graph is. That is the purpose of executing add_vertex() without parameters: to add a vertex numbered with the least nonnegative integer not already in the graph.

But the problem is that it doesn't return the number, so you can't add edges to it. Therefore add_vertex() w/o params currently is only useful for adding isolated vertices.

Apply trac_11739_add_vertex_returns_name.2.patch only.

[brunellus]

Sorry, wrong file. I'll send better.

[brunellus]

OK, that's it. I won't move the ticket to "needs review" now, because I'm not sure whether add_vertex should return the name of a vertex in a case the name was supplied as an argument. At first I thought it would be cleaner, but now when I had to rewrite tests I'm not sure, if it's practical.

So, do you have any advice?

[brunellus]

Oh, moreover, I'm little insconsisted now: when the addition of a new vertex silently fail, bipartite_graph.add_vertex returns None. That can be usefull, because the silent fail is still silent, but easily detectable. add_vertex of general_graph don't do this, but could do.

[brunellus]

OK, I thought it through and did it like ticket asks: add_vertex returns the name only if it was called with name=None.

[zimmerma]

I like this patch. I will run the doctests. However, shouldn't we do the same for add_vertices for consistency?

sage: G=Graph()
sage: G.add_vertex()
0
sage: G.add_vertices([None])

Note the following strange thing:

sage: G=Graph()
sage: G.add_vertices([None,0])
sage: G.vertices()
[0]

thus we added only one vertex.

Paul

[zimmerma]

I give a positive review, to enable progress on #9362. If you want to do the same for add_vertices, feel free to add it to this ticket, or open a separate ticket.

Paul

[jdemeyer]

You should format new documentation strings according to  http://sagemath.org/doc/developer/conventions.html#documentation-strings. In particular, the OUTPUT blocks are not properly formatted.

[zimmerma]

Jeroen, I don't see which OUTPUT blocks are not properly formatted, and why: should there be an empty line after the OUTPUT keyword? Should the block start just below the O from OUTPUT and not be indented? Please could you be more specific? It seems to me that the new blocks are in accordance with the previous ones in the corresponding files.

Paul

[jdemeyer]

Replying to zimmerma:

Jeroen, I don't see which OUTPUT blocks are not properly formatted, and why: should there be an empty line after the OUTPUT keyword? Should the block start just below the O from OUTPUT and not be indented? Please could you be more specific?

It should be like

OUTPUT:

foo if bla, bar otherwise.  If this is a long line, it
should be wrapped.

It seems to me that the new blocks are in accordance with the previous ones in the corresponding files.

I agree, but those are very old, probably from before we had standards.

[brunellus]

Replying to zimmerma:

However, shouldn't we do the same for add_vertices for consistency?

sage: G=Graph()
sage: G.add_vertex()
0
sage: G.add_vertices([None])

That could be useful, but there is this problem: what to do if there are no None-labeled vertices in the input? Returning an empty list is consistent, but in most cases, where the feature won't be used, this looks just weird and None would be much better:

sage: G.add_vertices([1, 3])
[]

Note the following strange thing:

sage: G=Graph()
sage: G.add_vertices([None,0])
sage: G.vertices()
[0]

thus we added only one vertex.

Good point. I guess it makes better sense to add named vertices first.

[zimmerma]

what to do if there are no None-labeled vertices in the input?

I would prefer that G.add_vertices([1, 3]) returns None.

Paul

[brunellus]

There are still few tests missing...

[zimmerma]

There are still few tests missing...

if you adressed the issues of comment 8 and you think the new patch is ready for review, you should tick the "needs review" box.

Paul

[brunellus]

So. Hope it's OK. :-)

[zimmerma]

positive review. All tests pass with Sage 4.8 (except a time out in sandpiles/sandpile.py which already happens without this patch).

Paul

PS: as a side note, we should homogeneize the name of the methods (vertices vs verts) in a separate ticket.