Opened 5 months ago

Closed 5 months ago

#26484 closed enhancement (fixed)

clean graph_coloring.py

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

Description (last modified by dcoudert)

Clean graph_coloring.py and avoid comparison of vertex labels.

Change History (9)

comment:1 Changed 5 months ago by dcoudert

  • Branch set to public/26484_graph_coloring_py
  • Cc tscrim chapoton added
  • Commit set to 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7
  • Description modified (diff)
  • Keywords py3 graph added
  • Status changed from new to needs_review

New commits:

9910b4dtrac #26484: clean graph_coloring.py

comment:2 Changed 5 months ago by dcoudert

  • Milestone changed from sage-8.4 to sage-8.5

comment:3 follow-up: Changed 5 months ago by tscrim

  • Reviewers set to 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 5 months ago by git

  • Commit changed from 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7 to 87f29c10f4bddcb704f3a8cb949a867862a260a5

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

87f29c1trac #26484: reviewers comments

comment:5 in reply to: ↑ 3 Changed 5 months ago by dcoudert

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

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

  • Commit changed from 87f29c10f4bddcb704f3a8cb949a867862a260a5 to f334d2b98724bba070c2ee3a18356fb56d5b075b

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 5 months ago by dcoudert

  • Status changed from needs_review to 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 5 months ago by vbraun

  • Branch changed from public/26484_graph_coloring_py to f334d2b98724bba070c2ee3a18356fb56d5b075b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.