Opened 4 years ago

Closed 4 years ago

#26484 closed enhancement (fixed)

clean graph_coloring.py

Reported by: David Coudert Owned by:
Priority: major Milestone: sage-8.5
Component: graph theory Keywords: py3, graph
Cc: Travis Scrimshaw, Frédéric Chapoton Merged in:
Authors: David Coudert Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: f334d2b (Commits, GitHub, GitLab) Commit: f334d2b98724bba070c2ee3a18356fb56d5b075b
Dependencies: Stopgaps:

Status badges

Description (last modified by David Coudert)

Clean graph_coloring.py and avoid comparison of vertex labels.

Change History (9)

comment:1 Changed 4 years ago by David Coudert

Branch: public/26484_graph_coloring_py
Cc: Travis Scrimshaw Frédéric Chapoton added
Commit: 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7
Description: modified (diff)
Keywords: py3 graph added
Status: newneeds_review

New commits:

9910b4dtrac #26484: clean graph_coloring.py

comment:2 Changed 4 years ago by David Coudert

Milestone: sage-8.4sage-8.5

comment:3 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: 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f787f29c10f4bddcb704f3a8cb949a867862a260a5

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

87f29c1trac #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: 87f29c10f4bddcb704f3a8cb949a867862a260a5f334d2b98724bba070c2ee3a18356fb56d5b075b

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

f334d2btrac #26484: reduce default number of tests in random_all_graph_colorings

comment:8 Changed 4 years ago by David Coudert

Status: needs_reviewpositive_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_pyf334d2b98724bba070c2ee3a18356fb56d5b075b
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.