Opened 8 years ago

Closed 8 years ago

#11739 closed enhancement (fixed)

add_vertex w/o params should return the new vertex

Reported by: ddestrada Owned by: jason, ncohen, rlm
Priority: minor Milestone: sage-5.0
Component: graph theory Keywords: add_vertex sd35.5
Cc: jason Merged in: sage-5.0.beta3
Authors: Lukáš Lánský Reviewers: Paul Zimmermann
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by zimmerma)

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.

Attachments (2)

trac_11739_add_vertex_returns_name.patch (6.3 KB) - added by brunellus 8 years ago.
trac_11739_add_vertex_returns_name.2.patch (13.4 KB) - added by brunellus 8 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by brunellus

Sorry, wrong file. I'll send better.

comment:2 Changed 8 years ago by 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?

comment:3 Changed 8 years ago by 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.

comment:4 Changed 8 years ago by jason

  • Cc jason added

Changed 8 years ago by brunellus

comment:5 Changed 8 years ago by brunellus

  • Status changed from new to needs_review

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

comment:6 follow-up: Changed 8 years ago by zimmerma

  • Keywords sd35.5 added

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

comment:7 Changed 8 years ago by zimmerma

  • Authors set to Lukáš Lánský
  • Reviewers set to Paul Zimmermann
  • Status changed from needs_review to positive_review

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

comment:8 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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.

comment:9 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

comment:10 follow-up: Changed 8 years ago by 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

comment:11 in reply to: ↑ 10 Changed 8 years ago by 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.

comment:12 in reply to: ↑ 6 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by 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

comment:14 Changed 8 years ago by brunellus

There are still few tests missing...

comment:15 Changed 8 years ago by 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

Changed 8 years ago by brunellus

comment:16 Changed 8 years ago by brunellus

  • Status changed from needs_work to needs_review

So. Hope it's OK. :-)

comment:17 Changed 8 years ago by zimmerma

  • Status changed from needs_review to positive_review

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.

comment:18 Changed 8 years ago by zimmerma

  • Description modified (diff)

comment:19 Changed 8 years ago by jdemeyer

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