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

[dcoudert]

clean the files (PEP8) and simplify some tests

CC: @tscrim

Component: graph theory

Author: David Coudert

Branch/Commit: 2cf89df

Reviewer: Travis Scrimshaw

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

[dcoudert]

Branch: public/26478_cleaning

[dcoudert]

Commit: 8c579b0

[dcoudert]

New commits:

bfe7c08	clean plot_graph_js.py
874eeb8	clean graph_list.py
8c579b0	clean graph_input.py

[dcoudert]

comment:2

The most significant changes are in graph_input.py as I changed some tests.

[tscrim]

comment:3

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.

[sagetrac-git]

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

39aa69d	trac #26478: reviewers comments in graph_input
3826f1e	trac #26478: reviewers comments in graph_list
90f89b0	trac #26478: reviewers comments in graph_plot_js

[sagetrac-git]

Changed commit from 8c579b0 to 90f89b0

[dcoudert]

comment:5

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 ?

[fchapoton]

comment:6

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..

[sagetrac-git]

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

93ea7bd

trac #26478: fix six import in graph_input.py

[sagetrac-git]

Changed commit from 90f89b0 to 93ea7bd

[dcoudert]

comment:8

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

[tscrim]

comment:10

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.

[sagetrac-git]

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

62403cd

trac #26478: fix doctests

[sagetrac-git]

Changed commit from 93ea7bd to 62403cd

[dcoudert]

comment:12

I fixed the doctests. Now waiting for the patchbot.

[tscrim]

comment:13

Thanks.

[tscrim]

Reviewer: Travis Scrimshaw

[vbraun]

comment:14

Merge conflict

[sagetrac-git]

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

2cf89df

trac #26478: fix merge conflict with 8.5.beta0

[sagetrac-git]

Changed commit from 62403cd to 2cf89df

[dcoudert]

comment:16

I fixed the merge conflict with 8.5.beta0

[vbraun]

Changed branch from public/26478_cleaning to 2cf89df