Opened 3 years ago

Closed 2 years ago

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

Reported by: Owned by: dcoudert major sage-8.5 graph theory py3, graph tscrim David Coudert Travis Scrimshaw N/A 2cf89df 2cf89df2339e0a1cb2e98fb0af9bc3ddeb0a993f

clean the files (PEP8) and simplify some tests

### comment:1 Changed 3 years ago by dcoudert

• Branch set to public/26478_cleaning

New commits:

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

### comment:2 Changed 3 years ago by dcoudert

• 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 3 years 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 3 years ago by git

• Commit changed from 8c579b08a5ac05ce2e4d840840bda79a5dad5654 to 90f89b08b54400fd66e9b62c625aa1b2c823f313

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`

### comment:5 Changed 3 years 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 3 years 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 3 years ago by git

• 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 3 years ago by dcoudert

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

Version 0, edited 3 years ago by dcoudert (next)

### comment:9 Changed 2 years ago by dcoudert

• Milestone changed from sage-8.4 to sage-8.5

### comment:10 Changed 2 years ago by tscrim

Doctest failures:

```File "src/sage/graphs/digraph.py", line 399, in sage.graphs.digraph.DiGraph
Failed example:
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 2 years ago by git

• 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 2 years ago by dcoudert

I fixed the doctests. Now waiting for the patchbot.

### comment:13 Changed 2 years ago by tscrim

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

Thanks.

### comment:14 Changed 2 years ago by vbraun

• Status changed from positive_review to needs_work

Merge conflict

### comment:15 Changed 2 years ago by git

• 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 2 years ago by dcoudert

• Status changed from needs_work to needs_review

I fixed the merge conflict with 8.5.beta0

### comment:17 Changed 2 years ago by tscrim

• Status changed from needs_review to positive_review

### comment:18 Changed 2 years 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.