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

clean digraph.py #26578

Closed
dcoudert opened this issue Oct 28, 2018 · 14 comments
Closed

clean digraph.py #26578

dcoudert opened this issue Oct 28, 2018 · 14 comments

Comments

@dcoudert
Copy link
Contributor

Mostly PEP8 cleaning and fixing the documentation.

Possible issue: sort lists of vertices in layout_acyclic_dummy.

CC: @tscrim @fchapoton

Component: graph theory

Author: David Coudert

Branch/Commit: e4be7c5

Reviewer: Travis Scrimshaw

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

@dcoudert dcoudert added this to the sage-8.5 milestone Oct 28, 2018
@dcoudert

This comment has been minimized.

@dcoudert
Copy link
Contributor Author

New commits:

43334c7clean digraph.py
7e257f2fix various issues in the documentation

@dcoudert
Copy link
Contributor Author

Branch: public/26578_digraph_py

@dcoudert
Copy link
Contributor Author

Commit: 7e257f2

@tscrim
Copy link
Collaborator

tscrim commented Oct 28, 2018

comment:2

This `i` divides `j`.:: will become $i$ divides $j$.:, so either remove the period or add a space before the ::.

if weighted is None: self._weighted = False should be on 2 lines.

Bad space: self.allow_loops(loops ,check=False).

Otherwise LGTM.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2018

Changed commit from 7e257f2 to 9814a1b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2018

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

9814a1btrac #26578: reviewer's comments

@dcoudert
Copy link
Contributor Author

comment:4

I have implemented your comments. On the way, I found some places where to use .edges(sort=False).

@tscrim
Copy link
Collaborator

tscrim commented Oct 30, 2018

comment:5

Thank you, although you've picked up some trivial failures:

sage -t --long src/sage/graphs/digraph_generators.py  # 4 doctests failed

Once fixed, you can set a positive review.

@tscrim
Copy link
Collaborator

tscrim commented Oct 30, 2018

Reviewer: Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 30, 2018

Changed commit from 9814a1b to e4be7c5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 30, 2018

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

e4be7c5trac #26578: fix doctests in digraph_generators.py

@dcoudert
Copy link
Contributor Author

comment:7

Thank you.

@vbraun
Copy link
Member

vbraun commented Oct 31, 2018

Changed branch from public/26578_digraph_py to e4be7c5

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