#26478 closed enhancement (fixed)

clean graph_plot_js.py, graph_list.py and graph_input.py

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.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 dcoudert)

clean the files (PEP8) and simplify some tests

Change History (18)

comment:1 Changed 13 months ago by dcoudert

  • Branch set to public/26478_cleaning
  • Commit set to 8c579b08a5ac05ce2e4d840840bda79a5dad5654

New commits:

bfe7c08clean plot_graph_js.py
874eeb8clean graph_list.py
8c579b0clean graph_input.py

comment:2 Changed 13 months ago by dcoudert

  • 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 13 months ago by tscrim

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 13 months ago by git

  • Commit changed from 8c579b08a5ac05ce2e4d840840bda79a5dad5654 to 90f89b08b54400fd66e9b62c625aa1b2c823f313

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

39aa69dtrac #26478: reviewers comments in graph_input
3826f1etrac #26478: reviewers comments in graph_list
90f89b0trac #26478: reviewers comments in graph_plot_js

comment:5 Changed 13 months ago by dcoudert

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 13 months ago by chapoton

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 13 months ago by git

  • Commit changed from 90f89b08b54400fd66e9b62c625aa1b2c823f313 to 93ea7bdd587ab946b3c68ebd9ef52d3407704ae0

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

93ea7bdtrac #26478: fix six import in graph_input.py

comment:8 Changed 13 months ago by dcoudert

Good to know. As I'm not Python 3 expert, I thought it could be something else.

Last edited 13 months ago by dcoudert (previous) (diff)

comment:9 Changed 13 months ago by dcoudert

  • Milestone changed from sage-8.4 to sage-8.5

comment:10 Changed 13 months ago by tscrim

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 13 months ago by git

  • Commit changed from 93ea7bdd587ab946b3c68ebd9ef52d3407704ae0 to 62403cd23042e6f8697dba2800648a2946a0919b

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

62403cdtrac #26478: fix doctests

comment:12 Changed 13 months ago by dcoudert

I fixed the doctests. Now waiting for the patchbot.

comment:13 Changed 13 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thanks.

comment:14 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:15 Changed 13 months ago by git

  • Commit changed from 62403cd23042e6f8697dba2800648a2946a0919b to 2cf89df2339e0a1cb2e98fb0af9bc3ddeb0a993f

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

2cf89dftrac #26478: fix merge conflict with 8.5.beta0

comment:16 Changed 13 months ago by dcoudert

  • Status changed from needs_work to needs_review

I fixed the merge conflict with 8.5.beta0

comment:17 Changed 13 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:18 Changed 13 months ago by vbraun

  • Branch changed from public/26478_cleaning to 2cf89df2339e0a1cb2e98fb0af9bc3ddeb0a993f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.