#13440: Adding reverse_edge() function to DiGraph
---------------------------------+------------------------------------------
Reporter: egunawan | Owner: jason, ncohen, rlm
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.7
Component: graph theory | Resolution:
Keywords: digraph | Work issues:
Report Upstream: N/A | Reviewers: Gregg Musiker, Nathann Cohen
Authors: Emily Gunawan | Merged in:
Dependencies: | Stopgaps:
---------------------------------+------------------------------------------
Changes (by ncohen):
* reviewer: Gregg Musiker => Gregg Musiker, Nathann Cohen
Old description:
> Adding reverse_edge() function to DiGraph
>
> Apply:
>
> * [attachment:trac_13440-reverse-edge.patch]
New description:
Adding reverse_edge() function to DiGraph
Apply:
* [attachment:trac_13440-reverse-edge.patch]
* [attachment:trac_13440-review.patch]
--
Comment:
Hellooooooooooooooooooooooooooooooooooooo !!
Well... Here is a patch (to be added on top of yours) that does several
modifications:
* When it does not make code harder to read, we try to keep lines of code
and documentation to a maximum length of 80 characters. I have no idea why
and I find that stupid, but people throw stones at you otherwise.
* Added some backticks around python keywords.
* Changed some explanations in the documentation
* Removed trailing whitespaces (we don't like that. At least that's what
I've been told.
I also changed some part of the method's behaviour, so tell me what you
think of it : my understanding is that setting "multiedges" to `False`
means "I'm ok with loss of information". Besides, here's the current
behaviour of `add_edge` in a graph with labels on the edges :
{{{
sage: g = Graph()
sage: g.add_edge(1,2,'label')
sage: g.edges()
[(1, 2, 'label')]
sage: g.add_edge(1,2,'labelllllll')
sage: g.edges()
[(1, 2, 'labelllllll')]
}}}
So I thought that it would make more sense, when `multiedges = False`, to
overwrite the label of edge `(1,2)` when `(2,1)` is to be reversed. Here's
an example I added in the docstring :
{{{
Note that in the following graph, specifying ``multiedges = False`` will
result in overwriting the label of `(1,2)` with the label of `(2,1)`::
sage: D = DiGraph( [(1, 2, 'B'), (2, 1,'A'), (2, 3, None)] )
sage: D.edges()
[(1, 2, 'B'), (2, 1, 'A'), (2, 3, None)]
sage: D.reverse_edge(2,1, multiedges=False)
sage: D.edges()
[(1, 2, 'B'), (2, 3, None)]
}}}
What do you think of it ?
If you agree with this second patch (and as I agree with yours), you can
set this ticket to `positive_review` `:-)`
And thank you for this patch... Even if I first complained about it. It's
good that it made me think a bit about the kind of functions a software
like Sage should contain `:-)`
Nathann
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13440#comment:33>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
