Opened 4 years ago

Closed 4 years ago

# clean graph_coloring.py

Reported by: Owned by: David Coudert major sage-8.5 graph theory py3, graph Travis Scrimshaw, Frédéric Chapoton David Coudert Travis Scrimshaw N/A f334d2b f334d2b98724bba070c2ee3a18356fb56d5b075b

Clean `graph_coloring.py` and avoid comparison of vertex labels.

### comment:1 Changed 4 years ago by David Coudert

Branch: → public/26484_graph_coloring_py Travis Scrimshaw Frédéric Chapoton added → 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7 modified (diff) py3 graph added new → needs_review

New commits:

 ​9910b4d `trac #26484: clean graph_coloring.py`

### comment:2 Changed 4 years ago by David Coudert

Milestone: sage-8.4 → sage-8.5

### comment:3 follow-up:  5 Changed 4 years ago by Travis Scrimshaw

Reviewers: → Travis Scrimshaw

A while-we-are-at-it:

```-    if not n: return
+    if not n:
+        return
-    if n < 0: raise ValueError("n must be non-negative")
+    if n < 0:
+        raise ValueError("n must be non-negative")
```

This is now a grammatical error:

```-    number of colors.
-
-    ::
+    number of colors ::
```

Remove the space before the `::` so it generates a colon after colors.

Can you use Python `set`'s here instead of Sage `Set`'s:

```            # make the dict into a set for quick uniqueness checking
S += Set([Set([(k,tuple(C[k])) for k in C])])
```

(or at least for intermediate computations)?

For this change:

```-            raise RuntimeError("It looks like you have found a counterexample to a very old conjecture. Please do not loose it ! Please publish it, and send a post to sage-devel to warn us. I implore you ! Nathann Cohen ")
+            raise RuntimeError("it looks like you have found a counterexample to a very old conjecture. Please do not loose it ! Please publish it, and send a post to sage-devel to warn us. I implore you ! Nathann Cohen ")
```

the `it` should be capitalized because the error message is so long and multiple sentences. Also, you should remove Nathann Cohen from the error message and say "We implore you!".

Everything else LGTM.

### comment:4 Changed 4 years ago by git

Commit: 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7 → 87f29c10f4bddcb704f3a8cb949a867862a260a5

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

 ​87f29c1 `trac #26484: reviewers comments`

### comment:5 in reply to:  3 Changed 4 years ago by David Coudert

Remove the space before the `::` so it generates a colon after colors.

I didn't know that. I'm learning every day. Cool.

Can you use Python `set`'s here instead of Sage `Set`'s:

Done. I also added a loop as the input parameter was not used

### comment:6 Changed 4 years ago by Travis Scrimshaw

Thanks. Actually, now I am a little worried about the default number of tests run in `random_all_graph_colorings`. It takes over a minute on my laptop. I think we are better having the default number be 10 or possibly 100. Once changed, you can set a positive review on my behalf.

### comment:7 Changed 4 years ago by git

Commit: 87f29c10f4bddcb704f3a8cb949a867862a260a5 → f334d2b98724bba070c2ee3a18356fb56d5b075b

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

 ​f334d2b `trac #26484: reduce default number of tests in random_all_graph_colorings`

### comment:8 Changed 4 years ago by David Coudert

Status: needs_review → positive_review

I reduced it to 2. Note that the doctest of that method calls it with 1.

Thanks for the review.

### comment:9 Changed 4 years ago by Volker Braun

Branch: public/26484_graph_coloring_py → f334d2b98724bba070c2ee3a18356fb56d5b075b → fixed positive_review → closed
Note: See TracTickets for help on using tickets.