Opened 23 months ago
Closed 23 months ago
#27668 closed enhancement (fixed)
py3: fix some doctests in French book (graph and programming)
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  python3  Keywords:  
Cc:  dcoudert  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  David Coudert 
Report Upstream:  N/A  Work issues:  
Branch:  a4e1bed (Commits, GitHub, GitLab)  Commit:  a4e1bed94546872411509738e272a690cd3b8627 
Dependencies:  Stopgaps: 
Description
Change History (14)
comment:1 Changed 23 months ago by
 Branch set to u/chapoton/27668
 Cc dcoudert added
 Commit set to 2f24d0bc55ee4a53121b47ff373f1e0522f9c09e
 Status changed from new to needs_review
comment:2 Changed 23 months ago by
The proposed patch fixes some doctests, but I also see this error.
File "src/sage/tests/books/computationalmathematicswithsagemath/graphtheory_doctest.py", line 386, in sage.tests.books.computationalmathematicswithsagemath.graphtheory_doctest Failed example: for i in tasks: print("t{} assigned to {}".format(i,M.neighbors('t'+str(i))[0])) Expected: t0 assigned to w2 t1 assigned to w3 t2 assigned to w5 t3 assigned to w8 t4 assigned to w1 t5 assigned to w7 t6 assigned to w9 t7 assigned to w0 t8 assigned to w4 t9 assigned to w6 Got: t0 assigned to w3 t1 assigned to w9 t2 assigned to w5 t3 assigned to w0 t4 assigned to w1 t5 assigned to w2 t6 assigned to w8 t7 assigned to w7 t8 assigned to w4 t9 assigned to w6
Do you want to solve it it or to postpone it for another ticket ?
comment:3 Changed 23 months ago by
postpone
comment:4 Changed 23 months ago by
 Reviewers set to David Coudert
 Status changed from needs_review to positive_review
So then, LGTM.
comment:5 Changed 23 months ago by
 Status changed from positive_review to needs_info
What's up with this?
@@ 280,8 +280,11 @@ Sage example in ./graphtheory.tex, line 1736:: Sage example in ./graphtheory.tex, line 1746:: sage: P = Permutations(range(g.order()))  sage: n_colors, coloration = min(  ....: greedy_coloring(g, P.random_element()) for i in range(50)) + sage: n_colors = g.order() + sage: for i in range(50): + ....: n_col, coloration = greedy_coloring(g, P.random_element()) + ....: if n_col < n_colors: + ....: n_colors = n_col sage: n_colors 4
It's not clear to me why the old code wouldn't work on Python 3. In any case this is an ugly way to write min(...)
so I would rather just skip this test if for some reason it doesn't work as a 1liner.
Please in the future CC me on changes to these tests.
comment:6 Changed 23 months ago by
this was comparing also the second argument "coloration" with itself, which is a dict..
comment:7 Changed 23 months ago by
min() takes a key
argument much like sorted
so you can take the min while comparing just n_colors
like min(..., key=lambda c: c[0])
, for example.
comment:8 Changed 23 months ago by
 Commit changed from 2f24d0bc55ee4a53121b47ff373f1e0522f9c09e to c8a1ab404602555ba05716bba376eb67812328aa
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c8a1ab4  py3: fix some doctests in French book

comment:9 Changed 23 months ago by
 Status changed from needs_info to needs_review
done, thx for the suggestion
comment:10 Changed 23 months ago by
 Commit changed from c8a1ab404602555ba05716bba376eb67812328aa to a4e1bed94546872411509738e272a690cd3b8627
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a4e1bed  py3: fix some doctests in French book

comment:11 Changed 23 months ago by
and the bot is now green
comment:12 Changed 23 months ago by
 Status changed from needs_review to positive_review
Right, it's much better like that.
comment:13 Changed 23 months ago by
+1 Thank you!
comment:14 Changed 23 months ago by
 Branch changed from u/chapoton/27668 to a4e1bed94546872411509738e272a690cd3b8627
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: fix some doctests in French book