Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize some methods for loops #22911

Closed
dcoudert opened this issue Apr 30, 2017 · 18 comments
Closed

Reorganize some methods for loops #22911

dcoudert opened this issue Apr 30, 2017 · 18 comments

Comments

@dcoudert
Copy link
Contributor

We have 2 methods for returning the loops of a (di)graph, loops and loop_edges, and one is faster than the other.

sage: G = Graph(10, loops=True)
sage: G.add_edges([(u,u) for u in G])
sage: %time L1 = G.loop_edges()
CPU times: user 68 µs, sys: 23 µs, total: 91 µs
Wall time: 73 µs
sage: %time L2 = G.loops()
CPU times: user 181 µs, sys: 50 µs, total: 231 µs
Wall time: 194 µs
sage: L1 == L2
True
sage: D = digraphs.DeBruijn(10,1)
sage: %time L1 = D.loop_edges()
CPU times: user 80 µs, sys: 18 µs, total: 98 µs
Wall time: 86.1 µs
sage: %time L2 = D.loops()
CPU times: user 333 µs, sys: 76 µs, total: 409 µs
Wall time: 344 µs
sage: L1 == L2
True

Note however that the slower (loops) has an extra parameter for edge labels. Let's try to clean that.

Component: graph theory

Author: David Coudert

Branch/Commit: fc65b8c

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/22911

@dcoudert dcoudert added this to the sage-8.0 milestone Apr 30, 2017
@dcoudert

This comment has been minimized.

@dcoudert
Copy link
Contributor Author

comment:1

So far I have let both methods and improved the loop_edges method.

I don't know what's the best option:

  1. deprecate one of the methods and ensure the fastest code is in the other one
  2. make the slowest method an alias for the fastest one and so let both names

Any advice / opinion is welcome. Option 2 is the easiest one, but...


New commits:

1521d8etrac #22911: reorder loop related methods
fd7bf7ftrac #22911: add parameter labels to loop_edges

@dcoudert
Copy link
Contributor Author

Branch: u/dcoudert/22911

@dcoudert
Copy link
Contributor Author

Commit: fd7bf7f

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2017

comment:2

I would not deprecate the method, but just make loops and alias for loop_edges (with the extra parameter).

Also, while you're moving methods around in the file (something I try to avoid doing because it can create trivial conflicts easily), I would clean up the docstrings Returns -> Return and

- ``labels`` -- whether returned edges have labels (``(u,v,l)``) or not (``(u,v)``)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

85b7ef5trac #22911: make loops an alias for loop_edges and fix doc strings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2017

Changed commit from fd7bf7f to 85b7ef5

@dcoudert
Copy link
Contributor Author

comment:4

Should be better now.

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2017

comment:5

Last thing, we should move those doctests from loops into loop_edges as they cover a few other cases now not tested (at least from looking at the diff).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

c22c505trac #22911: merge doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2017

Changed commit from 85b7ef5 to c22c505

@dcoudert
Copy link
Contributor Author

comment:7

I have merged the doctests. Some cases are may be not covered, but I don't know which one.

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2017

comment:8

Hmm...maybe I misparsed the graphs in my mind. Thanks. Just remove the period in the INPUT: and you can set a positive review on my behalf.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

fc65b8ctrac #22911: remove period in input

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2017

Changed commit from c22c505 to fc65b8c

@dcoudert
Copy link
Contributor Author

comment:10

Thank you Travis.

@vbraun
Copy link
Member

vbraun commented May 8, 2017

Changed branch from u/dcoudert/22911 to fc65b8c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants