Opened 20 months ago
Closed 19 months ago
#26478 closed enhancement (fixed)
clean graph_plot_js.py, graph_list.py and graph_input.py
Reported by:  dcoudert  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  graph theory  Keywords:  py3, graph 
Cc:  tscrim  Merged in:  
Authors:  David Coudert  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  2cf89df (Commits)  Commit:  2cf89df2339e0a1cb2e98fb0af9bc3ddeb0a993f 
Dependencies:  Stopgaps: 
Description (last modified by )
clean the files (PEP8) and simplify some tests
Change History (18)
comment:1 Changed 20 months ago by
 Branch set to public/26478_cleaning
 Commit set to 8c579b08a5ac05ce2e4d840840bda79a5dad5654
comment:2 Changed 20 months ago by
 Cc tscrim added
 Description modified (diff)
 Status changed from new to needs_review
The most significant changes are in graph_input.py
as I changed some tests.
comment:3 Changed 20 months ago by
For this change:
 if not loops and any(u in neighb for u,neighb in six.iteritems(M)):  if loops is False:  u = next(u for u,neighb in six.iteritems(M) if u in neighb)  raise ValueError("The graph was built with loops=False but input M has a loop at {}.".format(u))  loops = True  if loops is None:  loops = False + if any(not isinstance(M[u], dict) for u in M): + raise ValueError("input dict must be a consistent format") + + if not loops: + for u, neighb in six.iteritems(M): + if u in neighb: + if loops is False: + raise ValueError("the graph was built with loops=False but input M has a loop at {}".format(u)) + loops = True + break + if loops is None: + loops = False
the any
call is actually faster. Granted, your change will result in a faster error, but I think an error message being here is unexpected. Hence it is allowed to be slow. If someone really wants the extra speed and is using that test to separate cases, they should run their own test instead of catching the error (which is faster). So I am inclined to revert this.
Also, strictly speaking, the items for INPUT:
are not sentences and do not end in a period (although some of them are sufficiently long with multiple parts that require it). While I was letting this slide when it was previously there, the ones you are adding should not have it.
All of the other changes LGTM.
comment:4 Changed 20 months ago by
 Commit changed from 8c579b08a5ac05ce2e4d840840bda79a5dad5654 to 90f89b08b54400fd66e9b62c625aa1b2c823f313
comment:5 Changed 20 months ago by
The following lines are apparently incompatible with Python 3 (see patchbot).
+ if any(u in neighb for u,neighb in six.iteritems(M)):
+ u = next(u for u,neighb in six.iteritems(M) if u in neighb)
+ if any(u in neighb for u, neighb in six.iteritems(D)):
+ u = next(u for u, neighb in six.iteritems(M) if u in neighb)
Is there a special trick or should I turn these lines to for loops ?
comment:6 Changed 20 months ago by
No, no. It's just the patchbot prefering "iteritems" to "six.iteritems". Just change the imports accordingly.
And once again, it is never mandatory to have all green lights from the patchbot, as the patchbot is not smart..
comment:7 Changed 20 months ago by
 Commit changed from 90f89b08b54400fd66e9b62c625aa1b2c823f313 to 93ea7bdd587ab946b3c68ebd9ef52d3407704ae0
Branch pushed to git repo; I updated commit sha1. New commits:
93ea7bd  trac #26478: fix six import in graph_input.py

comment:8 Changed 20 months ago by
Good to know. As I'm not Python 3 expert, I though it could be something else.
comment:9 Changed 20 months ago by
 Milestone changed from sage8.4 to sage8.5
comment:10 Changed 20 months ago by
Doctest failures:
File "src/sage/graphs/digraph.py", line 399, in sage.graphs.digraph.DiGraph Failed example: D = DiGraph('IRAaDCIIOEOKcPWAo') Expected: Traceback (most recent call last): ... RuntimeError: The string (IRAaDCIIOEOKcPWAo) seems corrupt: for n = 10, the string is too short. Got: Traceback (most recent call last): ... RuntimeError: the string (IRAaDCIIOEOKcPWAo) seems corrupt: for n = 10, the string is too short
and similar in graph.py
. Once fixed and the patchbot comes back green, then it will be a positive review.
comment:11 Changed 20 months ago by
 Commit changed from 93ea7bdd587ab946b3c68ebd9ef52d3407704ae0 to 62403cd23042e6f8697dba2800648a2946a0919b
Branch pushed to git repo; I updated commit sha1. New commits:
62403cd  trac #26478: fix doctests

comment:12 Changed 20 months ago by
I fixed the doctests. Now waiting for the patchbot.
comment:13 Changed 20 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks.
comment:14 Changed 20 months ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:15 Changed 20 months ago by
 Commit changed from 62403cd23042e6f8697dba2800648a2946a0919b to 2cf89df2339e0a1cb2e98fb0af9bc3ddeb0a993f
Branch pushed to git repo; I updated commit sha1. New commits:
2cf89df  trac #26478: fix merge conflict with 8.5.beta0

comment:16 Changed 20 months ago by
 Status changed from needs_work to needs_review
I fixed the merge conflict with 8.5.beta0
comment:17 Changed 19 months ago by
 Status changed from needs_review to positive_review
comment:18 Changed 19 months ago by
 Branch changed from public/26478_cleaning to 2cf89df2339e0a1cb2e98fb0af9bc3ddeb0a993f
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
clean plot_graph_js.py
clean graph_list.py
clean graph_input.py