Opened 3 years ago
Closed 3 years ago
#26484 closed enhancement (fixed)
clean graph_coloring.py
Reported by:  dcoudert  Owned by:  

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  Commit:  f334d2b98724bba070c2ee3a18356fb56d5b075b 
Dependencies:  Stopgaps: 
Description (last modified by )
Clean graph_coloring.py
and avoid comparison of vertex labels.
Change History (9)
comment:1 Changed 3 years ago by
 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
comment:2 Changed 3 years ago by
 Milestone changed from sage8.4 to sage8.5
comment:3 followup: ↓ 5 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
A whileweareatit:
 if not n: return + if not n: + return  if n < 0: raise ValueError("n must be nonnegative") + if n < 0: + raise ValueError("n must be nonnegative")
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 sagedevel 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 sagedevel 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 3 years ago by
 Commit changed from 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7 to 87f29c10f4bddcb704f3a8cb949a867862a260a5
Branch pushed to git repo; I updated commit sha1. New commits:
87f29c1  trac #26484: reviewers comments

comment:5 in reply to: ↑ 3 Changed 3 years ago by
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 SageSet
's:
Done. I also added a loop as the input parameter was not used
comment:6 Changed 3 years ago by
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 3 years ago by
 Commit changed from 87f29c10f4bddcb704f3a8cb949a867862a260a5 to 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 3 years ago by
 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 3 years ago by
 Branch changed from public/26484_graph_coloring_py to f334d2b98724bba070c2ee3a18356fb56d5b075b
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #26484: clean graph_coloring.py