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:  sage8.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: 
Description (last modified by )
Clean graph_coloring.py
and avoid comparison of vertex labels.
Change History (9)
comment:1 Changed 4 years ago by
Branch:  → public/26484_graph_coloring_py 

Cc:  Travis Scrimshaw Frédéric Chapoton added 
Commit:  → 9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7 
Description:  modified (diff) 
Keywords:  py3 graph added 
Status:  new → needs_review 
comment:2 Changed 4 years ago by
Milestone:  sage8.4 → sage8.5 

comment:3 followup: 5 Changed 4 years ago by
Reviewers:  → 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 4 years ago by
Commit:  9910b4d5e58ba5e87e4e55e873cc77ba9f1d19f7 → 87f29c10f4bddcb704f3a8cb949a867862a260a5 

Branch pushed to git repo; I updated commit sha1. New commits:
87f29c1  trac #26484: reviewers comments

comment:5 Changed 4 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 4 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 4 years ago by
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
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
Branch:  public/26484_graph_coloring_py → f334d2b98724bba070c2ee3a18356fb56d5b075b 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
trac #26484: clean graph_coloring.py