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 )
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)
Change History (21)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
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
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
- Cc jason added
Changed 8 years ago by
comment:5 Changed 8 years ago by
- 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: ↓ 12 Changed 8 years ago by
- 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
- 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
- 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
- Milestone changed from sage-4.8 to sage-5.0
comment:10 follow-up: ↓ 11 Changed 8 years ago by
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
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
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
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
There are still few tests missing...
comment:15 Changed 8 years ago by
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
comment:16 Changed 8 years ago by
- Status changed from needs_work to needs_review
So. Hope it's OK. :-)
comment:17 Changed 8 years ago by
- 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
- Description modified (diff)
comment:19 Changed 8 years ago by
- Merged in set to sage-5.0.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
Sorry, wrong file. I'll send better.