Opened 5 years ago
Last modified 4 years ago
#23572 closed task
doctests for the english translation of the book "Calcul mathématique avec Sage" — at Version 99
Reported by:  Paul Zimmermann  Owned by:  Frédéric Chapoton 

Priority:  critical  Milestone:  sage8.7 
Component:  doctest coverage  Keywords:  
Cc:  Marcelo Forets, David Coudert, Nicolas M. Thiéry, vklein, John Palmieri  Merged in:  
Authors:  Erik Bray  Reviewers:  Erik Bray 
Report Upstream:  Reported upstream. No feedback yet.  Work issues:  support `reduce` as a global in Python 3 
Branch:  public/french_book_python3 (Commits, GitHub, GitLab)  Commit:  b32170fe754f9293b8c7324414511c61f12ce64b 
Dependencies:  #26656  Stopgaps: 
Description (last modified by )
The purpose of this ticket is to add the doctests corresponding to the examples given in the english translation of the book "Calcul mathématique avec Sage" (see https://members.loria.fr/PZimmermann/sagebook/english.html).
Sage already includes doctests for the original version of this book (published in french in 2013). These doctests are in src/sage/tests/french_book
, and should be updated.
The final version of the book is now under press, so it is time for adding/updating doctests.
NOTE: some doctests are not exactly those from the book. They have been modified (in a minimal manner) so that they will pass when sage is run using python 3.
bugs found:
 Using
scipy.sparse.lil_matrix
is now broken: #23867
Change History (99)
comment:1 Changed 5 years ago by
Cc:  Marcelo Forets added 

comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
another issue:
┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 8.0, Release Date: 20170721 │ │ Type "notebook()" for the browserbased notebook interface. │ │ Type "help()" for help. │ └────────────────────────────────────────────────────────────────────┘ sage: C = graphs.ChvatalGraph() sage: from sage.graphs.graph_coloring import edge_coloring sage: edge_coloring(C, hex_colors = True) {'#00ffff': [(0, 6), (1, 5), (2, 8), (3, 4), (7, 11), (9, 10)], '#7f00ff': [(0, 4), (1, 7), (2, 6), (3, 9), (5, 11), (8, 10)], '#7fff00': [(0, 9), (1, 2), (3, 7), (4, 8), (5, 10), (6, 11)], '#ff0000': [(0, 1), (2, 3), (4, 5), (6, 10), (7, 8), (9, 11)]}
With Sage 8.1beta3 we get:
{'#00ffff': [(0, 9), (1, 5), (2, 6), (3, 4), (7, 11), (8, 10)], '#7f00ff': [(0, 1), (2, 8), (3, 7), (4, 5), (6, 10), (9, 11)], '#7fff00': [(0, 4), (1, 2), (3, 9), (5, 10), (6, 11), (7, 8)], '#ff0000': [(0, 6), (1, 7), (2, 3), (4, 8), (5, 11), (9, 10)]}
Is there a way to get a deterministic answer? Is there any random seed that controls the edge coloring algorithm?
comment:4 Changed 5 years ago by
comment:5 Changed 5 years ago by
Cc:  David Coudert added 

comment:6 Changed 5 years ago by
The coloring is done with ILP and the result depends on the ILP solver. #22564 has been merged in 8.1.beta?? The patch only slightly changes the order in which constraints are added to the ILP and it is enough the get a different solution from 8.0.
sage: from sage.graphs.graph_coloring import edge_coloring sage: edge_coloring(C, hex_colors = True, solver='GLPK') {'#00ffff': [(0, 9), (1, 5), (2, 6), (3, 4), (7, 11), (8, 10)], '#7f00ff': [(0, 1), (2, 8), (3, 7), (4, 5), (6, 10), (9, 11)], '#7fff00': [(0, 4), (1, 2), (3, 9), (5, 10), (6, 11), (7, 8)], '#ff0000': [(0, 6), (1, 7), (2, 3), (4, 8), (5, 11), (9, 10)]} sage: edge_coloring(C, hex_colors = True, solver='Cplex') {'#00ffff': [(0, 6), (1, 5), (2, 3), (4, 8), (7, 11), (9, 10)], '#7f00ff': [(0, 1), (2, 6), (3, 4), (5, 10), (7, 8), (9, 11)], '#7fff00': [(0, 9), (1, 2), (3, 7), (4, 5), (6, 11), (8, 10)], '#ff0000': [(0, 4), (1, 7), (2, 8), (3, 9), (5, 11), (6, 10)]} sage: edge_coloring(C, hex_colors = True, solver='PPL') {'#00ffff': [(0, 9), (1, 5), (2, 8), (3, 4), (6, 10), (7, 11)], '#7f00ff': [(0, 6), (1, 7), (2, 3), (4, 5), (8, 10), (9, 11)], '#7fff00': [(0, 4), (1, 2), (3, 9), (5, 10), (6, 11), (7, 8)], '#ff0000': [(0, 1), (2, 6), (3, 7), (4, 8), (5, 11), (9, 10)]} sage: edge_coloring(C, hex_colors = True, solver='Coin') {'#00ffff': [(0, 4), (1, 5), (2, 6), (3, 9), (7, 11), (8, 10)], '#7f00ff': [(0, 6), (1, 2), (3, 7), (4, 8), (5, 10), (9, 11)], '#7fff00': [(0, 1), (2, 3), (4, 5), (6, 11), (7, 8), (9, 10)], '#ff0000': [(0, 9), (1, 7), (2, 8), (3, 4), (5, 11), (6, 10)]} sage: edge_coloring(C, hex_colors = True, solver='Gurobi') {'#00ffff': [(0, 1), (2, 6), (3, 9), (4, 5), (7, 11), (8, 10)], '#7f00ff': [(0, 6), (1, 7), (2, 3), (4, 8), (5, 10), (9, 11)], '#7fff00': [(0, 4), (1, 5), (2, 8), (3, 7), (6, 11), (9, 10)], '#ff0000': [(0, 9), (1, 2), (3, 4), (5, 11), (6, 10), (7, 8)]}
In doctests, you must set the solver to GLPK
. Otherwise, the default solver is used (e.g., Cplex
if you have it).
comment:7 followup: 8 Changed 5 years ago by
David,
1) from the trac page, #22564 has been merged in 8.0, thus the difference I see should be due to subsequent changes
2) is there any reason why the order of the constraints was changed? It makes it difficult to add doctests for this example from our book about Sage
comment:8 Changed 5 years ago by
Replying to zimmerma:
David,
1) from the trac page, #22564 has been merged in 8.0, thus the difference I see should be due to subsequent changes
2) is there any reason why the order of the constraints was changed? It makes it difficult to add doctests for this example from our book about Sage
I changed the code 1) to split the graph into connected components and color them separately (should be faster), and 2) because I don't understand why we should use list comprehension for adding constraints. It was:
# A vertex can not have two incident edges with the same color. [p.add_constraint( p.sum([color[R(e),i] for e in g.edges_incident(v, labels=False)]), max=1) for v in g.vertex_iterator() for i in range(k)] # an edge must have a color [p.add_constraint(p.sum([color[R(e),i] for i in range(k)]), max=1, min=1) for e in g.edge_iterator(labels=False)] # anything is good as an objective value as long as it is satisfiable e = next(g.edge_iterator(labels=False)) p.set_objective(color[R(e),0])
Now it is:
# A vertex can not have two incident edges with the same color. for v in h.vertex_iterator(): for i in range(k): p.add_constraint(p.sum(color[R(u,v),i] for u in h.neighbor_iterator(v)) <= 1) # Nn edge must have a color for u,v in h.edge_iterator(labels=False): p.add_constraint(p.sum(color[R(u,v),i] for i in range(k)) == 1) # We color the edges of the vertex of maximum degree for i,v in enumerate(h.neighbors(X)): p.add_constraint( color[R(v,X),i] == 1 )
So the main changes are:
 use of
neighbor_iterator
instead ofedges_incident
in the first set of constraints (it has always been assumed in this method that the graph is simple)  removal of the objective function (not needed)
 now force the coloring of the edges incident to the vertex of maximum degree (could speed up the resolution).
In fact, I bet that changing the version of GLPK
could already change the result (e.g., faster algorithm converging to a different optimal solution). Furthermore, I assume that some internal heuristics of the solvers are randomized since we can set random seed of Cplex.
comment:9 Changed 5 years ago by
David, thank you for the explanation (by the way there is a typo: "Nn edge must have a color" should be "An edge...").
Since it seems impossible to have a reliable doctest for this example, we won't test it.
comment:10 Changed 5 years ago by
Replying to zimmerma:
Sage already includes doctests for the original version of this book (published in french in 2013). These doctests are in
src/sage/tests/french_book
, and should be updated.
Didn't you mention at some point that you didn't care any more about these doctests? I seem to remember something along those lines...
comment:11 Changed 5 years ago by
Jeroen,
what I said is that there were too many changes in Sage upstream (with respect to Sage 5.9 which we used in the french book in 2013) so that we could check them (and even, sometimes, we had no chance to give our opinion on them before they were merged into Sage). Those doctests nevertheless did they job since most of the examples still run with current versions of Sage.
Now that we have updated the book to Sage 8.0, we'd like to add the (new) doctests to Sage doctests, so that the new (english) version of the book will not be obsolete too quickly.
comment:12 followups: 13 14 15 Changed 5 years ago by
Nicolas Thiery just tested the doctests of our book with Sage 8.1beta5 and several do fail (while all tests pass with Sage 8.0). Here are the issues:
1) the lil_matrix
do not work any more. Cf the example on page 299 of the english translation:
sage: from scipy.sparse.linalg.dsolve import * sage: from scipy.sparse import lil_matrix sage: from numpy import array sage: n = 200 sage: n2 = n*n sage: A = lil_matrix((n2, n2)) sage: h2 = 1./float((n+1)^2) sage: for i in range(0,n2): ....: A[i,i]=4*h2+1. ....: if i+1<n2: A[i,int(i+1)]=h2 ....: if i>0: A[i,int(i1)]=h2 ....: if i+n<n2: A[i,int(i+n)]=h2 ....: if in>=0: A[i,int(in)]=h2 sage: Acsc = A.tocsc() sage: b = array([1 for i in range(0,n2)]) sage: solve = factorized(Acsc) # LU factorisation sage: S = solve(b) # resolution
Now if fails at the A = lil_matrix((n2, n2))
line with:
TypeError: unrecognized lil_matrix constructor usage
Aren't lil_matrix
tested in the current doctests?
2) the category of QQ
changed:
Failed example: QQ.category() Expected: Join of Category of quotient fields and Category of metric spaces Got: Join of Category of number fields and Category of quotient fields and Category of metric spaces
3) we have different output for graphs of the commands minor
and edge_coloring
. We will try to fix those with solver='GLPK'
(cf comment 6 above). If that does not work, is there a random seed that we can set to make those commands deterministic?
comment:13 followup: 18 Changed 5 years ago by
Replying to zimmerma:
3) we have different output for graphs of the commands
minor
andedge_coloring
. We will try to fix those withsolver='GLPK'
(cf comment 6 above). If that does not work, is there a random seed that we can set to make those commands deterministic?
Are these tests really nondeterministic or just different with different versions of Sage?
comment:14 followup: 19 Changed 5 years ago by
Replying to zimmerma:
2) the category of
Failed example: QQ.category() Expected: Join of Category of quotient fields and Category of metric spaces Got: Join of Category of number fields and Category of quotient fields and Category of metric spaces
This is a good and intentional change: just change the expected output.
comment:15 Changed 5 years ago by
Replying to zimmerma:
Aren't
lil_matrix
tested in the current doctests?
lil_matrix
is not used or tested anywhere in Sage. I'll have a look.
comment:16 Changed 5 years ago by
1) the lil_matrix do not work any more.
a scipy update? in sage 8.0.beta5 the version is 0.19.1, while in 8.0 it is 0.17.1.
however, the docstring says it's still valid:
... lil_matrix((M, N), [dtype]) to construct an empty matrix with shape (M, N) dtype is optional, defaulting to dtype='d'.
comment:17 Changed 5 years ago by
Description:  modified (diff) 

Report Upstream:  N/A → Reported upstream. No feedback yet. 
comment:18 Changed 5 years ago by
Replying to jdemeyer:
Replying to zimmerma:
3) we have different output for graphs of the commands
minor
andedge_coloring
. We will try to fix those withsolver='GLPK'
(cf comment 6 above). If that does not work, is there a random seed that we can set to make those commands deterministic?Are these tests really nondeterministic or just different with different versions of Sage?
no idea, I first asked Nicolas to test with solver='GLPK'
.
comment:19 followup: 20 Changed 5 years ago by
Replying to jdemeyer:
Replying to zimmerma:
2) the category of
Failed example: QQ.category() Expected: Join of Category of quotient fields and Category of metric spaces Got: Join of Category of number fields and Category of quotient fields and Category of metric spacesThis is a good and intentional change: just change the expected output.
this is not possible, since we currently use Sage 8.0 for the book, and this will make the doctests fail with 8.0.
comment:20 Changed 5 years ago by
Replying to zimmerma:
this is not possible, since we currently use Sage 8.0 for the book, and this will make the doctests fail with 8.0.
Is there any particular reason that you focus on 8.0? Given that the book is still in preparation, you could use Sage 8.1 instead.
comment:21 Changed 5 years ago by
Description:  modified (diff) 

comment:22 Changed 5 years ago by
Description:  modified (diff) 

comment:24 followup: 25 Changed 5 years ago by
General comment: all the doctests in the book which return types will change in Python 3. Python 2 uses <type 'int'>
while Python 3 uses <class 'int'>
.
comment:25 Changed 5 years ago by
Replying to jdemeyer:
General comment: all the doctests in the book which return types will change in Python 3. Python 2 uses
<type 'int'>
while Python 3 uses<class 'int'>
.
thanks Jeroen. Alas, there is nothing we can do for that, just wait for Python 3 and update.
comment:26 followup: 28 Changed 5 years ago by
For Sage doctests, you should write <... 'int'>
instead of <type 'int'>
.
comment:28 Changed 4 years ago by
Replying to jdemeyer:
For Sage doctests, you should write
<... 'int'>
instead of<type 'int'>
.
This actually isn't necessary; the doctest parser already accounts for this minor difference.
comment:30 Changed 4 years ago by
Milestone:  sage8.1 → sage8.4 

Priority:  major → critical 
Getting this done ASAP before 8.4 is out is pretty important, especially to make sure there's enough time to fix any failures that might still be lingering.
comment:31 Changed 4 years ago by
Authors:  → Erik Bray 

Branch:  → u/embray/ticket23572 
Commit:  → 7f3490eba3cd65e5a3e90580cd8d040a04040bc8 
Current versions of the examples/exercises. There are still about half a dozen test failures, all of which are insubstantial and most of which stem from #25247, but will still need to be fixed. The question is whether to fix them in the book, in the test suite, or both.
For one of them I'm getting a deprecation warning from scipy that we should probably heed:
sage t long src/sage/tests/books/sagebook/linsolve_doctest.py ********************************************************************** File "src/sage/tests/books/sagebook/linsolve_doctest.py", line 385, in sage.tests.books.sagebook.linsolve_doctest Failed example: x = cg(A, b, M = msc, tol=1.e8) Expected nothing Got: doctest:warning ... : DeprecationWarning: scipy.sparse.linalg.cg called without specifying `atol`. The default value will be changed in a future release. For compatibility, specify a value for `atol` explicitly, e.g., ``cg(..., atol=0)``, or to retain the old behavior ``cg(..., atol='legacy')``
New commits:
7f3490e  replace sage/tests/french_book with sage/tests/books/sagebook, containing

comment:32 Changed 4 years ago by
Status:  new → needs_info 

comment:33 Changed 4 years ago by
Erik,
1) the examples of the book are now updated to Sage 8.3, and all examples pass with Sage 8.3
2) about the conjugate gradient failure, there is no atol
option in Sage 8.3, only tol
,
and using atol=0
or atol='legacy'
gives an error in Sage 8.3:
sage: x = cg(A, b, M = msc, tol=1.e8, atol=0) ... TypeError: cg() got an unexpected keyword argument 'atol'
The only solution I see is to ignore this warning in the doctests. How can we do this?
comment:34 Changed 4 years ago by
I'm not sure what to do about thisI don't recall what version of scipy is included by default with Sage 8.3, but in 8.4.beta5 it's now scipy 1.1.0, which wants you to specify atol
.
I could have the test ignore that warning, but it would be better to just include a fixed version of that example in Sage. The question is hhether to update the example in the book as well, or to just manually edit that one test in the copy included in the Sage tests.
comment:36 Changed 4 years ago by
We can't change the example in the book since it should work with Sage 8.3.
I suggest you modify the doctest in the Sage distribution, by adding atol='legacy'
.
This should solve this issue.
comment:37 followup: 38 Changed 4 years ago by
So, to answer the more general question, if there are any examples from the book that no longer work exactly on Sage 8.4, I should copy them from the book asis, and then only modify the copies that are added to Sage by this ticket?
I ask because if any of the examples need to be modified, one will eventually want to get them back into the book for a future version.
comment:38 Changed 4 years ago by
Replying to embray:
So, to answer the more general question, if there are any examples from the book that no longer work exactly on Sage 8.4, I should copy them from the book asis, and then only modify the copies that are added to Sage by this ticket?
yes. But please tell me of any such examples. Maybe we can still modify them in the book so that they work both for 8.3 and 8.4 (this is unfortunately not possible for the atol
issue above).
comment:39 Changed 4 years ago by
Perhaps I'll just create a branch on the book repo for now to add any relevant updates.
comment:40 Changed 4 years ago by
For completeness, these are the current failures I get against 8.4.beta6:
sage t src/sage/tests/books/sagebook/integration_doctest.py ********************************************************************** File "src/sage/tests/books/sagebook/integration_doctest.py", line 109, in sage.tests.books.sagebook.integration_doctest Failed example: gp('intnum(x=0, 1, x^(99/100))') # abs tol 1e16 Expected: 73.62914262423378365 Got: 73.629142624233783843668417691077783339 Tolerance exceeded: 73.62914262423378365 vs 73.629142624233783843668417691077783339, tolerance 2e16 > 1e16 ********************************************************************** File "src/sage/tests/books/sagebook/integration_doctest.py", line 119, in sage.tests.books.sagebook.integration_doctest Failed example: gp('intnum(x=[0, 1/42], 1, x^(99/100))') # abs tol 1e16 Expected: 74.47274932028288503 Got: 74.472749320282885192304428135608736736 Tolerance exceeded: 74.47274932028288503 vs 74.472749320282885192304428135608736736, tolerance 2e16 > 1e16 ********************************************************************** 1 item had failures: 2 of 87 in sage.tests.books.sagebook.integration_doctest [86 tests, 2 failures, 23.25 s] sage t src/sage/tests/books/sagebook/polynomes_doctest.py ********************************************************************** File "src/sage/tests/books/sagebook/polynomes_doctest.py", line 240, in sage.tests.books.sagebook.polynomes_doctest Failed example: r.reduce(); r Expected: 1.00000000000000/(x + 1.00000000000000) Got: 1.00000000000000/(x  1.00000000000000) ********************************************************************** 1 item had failures: 1 of 111 in sage.tests.books.sagebook.polynomes_doctest [110 tests, 1 failure, 2.23 s] sage t src/sage/tests/books/sagebook/sol/integration_doctest.py ********************************************************************** File "src/sage/tests/books/sagebook/sol/integration_doctest.py", line 39, in sage.tests.books.sagebook.sol.integration_doctest Failed example: N(QuadNC(lambda x: x * log(1+x), 0, 1, 19)) # abs tol 1e15 Expected: 0.250000000000001 Got: 0.249999999999999 Tolerance exceeded: 0.250000000000001 vs 0.249999999999999, tolerance 2e15 > 1e15 File "src/sage/tests/books/sagebook/numbertheory_doctest.py", line 34, in sage.tests.books.sagebook.numbertheory_doctest Failed example: 1/a Expected: Traceback (most recent call last): ... ZeroDivisionError: Inverse does not exist. Got: <BLANKLINE> Traceback (most recent call last): ... ZeroDivisionError: inverse of Mod(3, 15) does not exist
Most of them seem to be caused by slight changes in PARI, though I'm not sure about the one with polynomes; obviously it's just choosing for some reason not to distribute the minus sign.
The last one is because the old exception message was slightly enhanced.
comment:41 followup: 42 Changed 4 years ago by
Erik, I fixed the error tolerances in the book. Please could you check again?
For the failure in polynomes, is there a way in the doctests to check we get one of both outputs?
For the numbertheory failure, it is unfortunate the error message did change. Is there a way to check for "ZeroDivisionError?: ... does not exist" in the doctest?
comment:42 Changed 4 years ago by
Replying to zimmerma:
Erik, I fixed the error tolerances in the book. Please could you check again?
I will double check, but yesterday I pulled down the latest version of the book, rebuilt it, and recopied the files. I can try doing it one more time, perhaps with with a clean of the repository first...
For the failure in polynomes, is there a way in the doctests to check we get one of both outputs?
Theoretically you could check for both, but it would make for a rather ugly test like:
sage: repr(r) in ['1.00000000000000/(x + 1.00000000000000)', '1.00000000000000/(x  1.00000000000000)'] True
Rather than checking for one of both it would be better to simply check the result by value like:
sage: r == 1.0/(x  1.0)
or something like that. There might also be a way to normalize it; I'll take a close look.
For the numbertheory failure, it is unfortunate the error message did change. Is there a way to check for "ZeroDivisionError?: ... does not exist" in the doctest?
I don't think it's that "unfortunate". It did change for the better, and in 8.4, whereas I had thought the book was still aiming specifically at 8.3. While having these tests in the Sage test suite is an important way to keep track of how our changes impact real use cases, it should not block useful changes that happen to deviate from the book (especially in trivial ways).
I don't see any point in changing the doctest in Sage to be less specific. If you want to change it in the book, ZeroDivisionEror: ...does not exist
would work, but is maybe a bit ugly and uninformative...?
comment:43 followup: 44 Changed 4 years ago by
Erik,
for the "polynomes" failure, I have added the doctest you suggest with repr
(the second one you propose also works before r.reduce()
, thus is not enough).
For the "numbertheory" failure, I have added "test=false", so this example is not tested any more.
All doctests still work with Sage 8.3.
Please could you check they all work with 8.4beta6?
Thank you.
comment:44 Changed 4 years ago by
Replying to zimmerma:
Erik,
for the "polynomes" failure, I have added the doctest you suggest with
repr
(the second one you propose also works beforer.reduce()
, thus is not enough).
I really wouldn't.... My whole point was it was not a good test, and even worse as an example in a book. Let me see if there is a better way to write this example; I just haven't tried yet.
For the "numbertheory" failure, I have added "test=false", so this example is not tested any more.
All doctests still work with Sage 8.3.
Please could you check they all work with 8.4beta6?
That's what I'm currently testing against...
comment:45 Changed 4 years ago by
Sorry, the above was me. Forgot I was still logged in as my bot :)
comment:46 Changed 4 years ago by
Okay, I doublechecked and as I suspected I am up to date with the book files, and still get slight floating point difference in the tests I mentioned previously. I wonder if there has been a PARI or Cypari update since 8.3.
Anyways, this diff is all I need for the tests to pass:

src/sage/tests/books/sagebook/integration_doctest.py
diff git a/src/sage/tests/books/sagebook/integration_doctest.py b/src/sage/tests/books/sagebook/integration_doctest.py index 4f2699c..1a7c610 100644
a b Sage example in ./integration.tex, line 717:: 106 106 Sage example in ./integration.tex, line 745:: 107 107 108 108 sage: p = gp.set_precision(old_prec) # we reset the default precision 109 sage: gp('intnum(x=0, 1, x^(99/100))') # abs tol 1e16109 sage: gp('intnum(x=0, 1, x^(99/100))') # abs tol 2e16 110 110 73.62914262423378365 111 111 112 112 Sage example in ./integration.tex, line 753:: … … Sage example in ./integration.tex, line 753:: 116 116 117 117 Sage example in ./integration.tex, line 764:: 118 118 119 sage: gp('intnum(x=[0, 1/42], 1, x^(99/100))') # abs tol 1e16119 sage: gp('intnum(x=[0, 1/42], 1, x^(99/100))') # abs tol 2e16 120 120 74.47274932028288503 121 121 122 122 Sage example in ./integration.tex, line 783:: 
src/sage/tests/books/sagebook/linsolve_doctest.py
diff git a/src/sage/tests/books/sagebook/linsolve_doctest.py b/src/sage/tests/books/sagebook/linsolve_doctest.py index 552bfc2..812b6b7 100644
a b Sage example in ./linsolve.tex, line 2721:: 382 382 ....: m[i,i] = 1./A[i,i] 383 383 sage: msc = m.tocsc() 384 384 sage: from scipy.sparse.linalg import cg 385 sage: x = cg(A, b, M = msc,tol=1.e8)385 sage: x = cg(A, b, M=msc, atol=1.e8) 386 386 387 387 Sage example in ./linsolve.tex, line 2782:: 388 388 
src/sage/tests/books/sagebook/numbertheory_doctest.py
diff git a/src/sage/tests/books/sagebook/numbertheory_doctest.py b/src/sage/tests/books/sagebook/numbertheory_doctest.py index cb8e331..09b291d 100644
a b Sage example in ./numbertheory.tex, line 174:: 34 34 sage: 1/a 35 35 Traceback (most recent call last): 36 36 ... 37 ZeroDivisionError: Inverse does not exist.37 ZeroDivisionError: inverse of Mod(3, 15) does not exist 38 38 39 39 Sage example in ./numbertheory.tex, line 199:: 40 40 
src/sage/tests/books/sagebook/polynomes_doctest.py
diff git a/src/sage/tests/books/sagebook/polynomes_doctest.py b/src/sage/tests/books/sagebook/polynomes_doctest.py index 926922d..77ba536 100644
a b Sage example in ./polynomes.tex, line 1399:: 238 238 Sage example in ./polynomes.tex, line 1411:: 239 239 240 240 sage: r.reduce(); r 241 1.00000000000000/(x +1.00000000000000)241 1.00000000000000/(x  1.00000000000000) 242 242 243 243 Sage example in ./polynomes.tex, line 1487:: 244 244 
src/sage/tests/books/sagebook/sol/integration_doctest.py
diff git a/src/sage/tests/books/sagebook/sol/integration_doctest.py b/src/sage/tests/books/sagebook/sol/integration_doctest.py index 518dcc9..b2c2e68 100644
a b Sage example in ./sol/integration.tex, line 58:: 36 36 37 37 sage: numerical_integral(x * log(1+x), 0, 1) 38 38 (0.25, 2.7755575615628914e15) 39 sage: N(QuadNC(lambda x: x * log(1+x), 0, 1, 19)) # abs tol 1e1539 sage: N(QuadNC(lambda x: x * log(1+x), 0, 1, 19)) # abs tol 2e15 40 40 0.250000000000001 41 41 sage: numerical_integral(sqrt(1x^2), 0, 1) 42 42 (0.785398167726482..., 9.042725224567119...e07)
I still think the best thing to do is to apply that just in the sage doctests, and then duplicate it to a branch in the book for future updates.
comment:47 followup: 48 Changed 4 years ago by
Erik, the abs tol
changes were already applied in the book sources, thus you should not need them any more.
As I told before, we cannot apply the atol
change since the example would not work any more with Sage 8.3, which is the version we use in the book (and the latest release at that time). I have just added test=false
to that example, thus it should not appear any more in the doctests.
For the numbertheory issue, I have also put test=false
in the book sources, thus it should no longer be tested.
And finally for the polynomes issue, I have replaced the test by the one using repr(r)
that you suggested (in silent mode).
In summary, if you regenerate the doctests with revision 739453a of the book sources, all doctests should pass with both 8.3 and 8.4beta. Please can you confirm?
comment:48 Changed 4 years ago by
Replying to zimmerma:
Erik, the
abs tol
changes were already applied in the book sources, thus you should not need them any more.
Ok. I see you did that yesterday, after I already doublechecked.
As I told before, we cannot apply the
atol
change since the example would not work any more with Sage 8.3, which is the version we use in the book (and the latest release at that time). I have just addedtest=false
to that example, thus it should not appear any more in the doctests.
If you say so. My point is just that it's a test that can work fine when modified. Perhaps it's not worth testing though since it's just a scipy function.
For the numbertheory issue, I have also put
test=false
in the book sources, thus it should no longer be tested.And finally for the polynomes issue, I have replaced the test by the one using
repr(r)
that you suggested (in silent mode).
That helps, but I still don't think it's a good way to test this.
comment:49 Changed 4 years ago by
I guess, my point is, if there's a [silent]
copy of that test in the book source, solely for the purpose of generating doctests to into Sage's test suite, then it might as well just be the version of the test that works on Sage 8.4. There's no reason at all for it to work on 8.3.
comment:50 Changed 4 years ago by
To be clear, the result here should be deterministic. Looking at the diffs, there was some work done in this area recently.
comment:51 Changed 4 years ago by
Commit:  7f3490eba3cd65e5a3e90580cd8d040a04040bc8 → d8014819b4a93c41c5ef0cdd73a8851b11806a07 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d801481  replace sage/tests/french_book with sage/tests/books/sagebook, containing

comment:52 Changed 4 years ago by
Status:  needs_info → needs_review 

Well, I went ahead and updated this from the latest version of the book, and now all tests pass, so let's just go with it. I am still confused, Paul, about what exactly your longterm intentions are w.r.t. keeping the book+sage+doctests all in sync. But this is good enough for now.
Volker, if we could have this merged in 8.4 that would be ideal. If for some reason it has to wait, I will do my best to ensure that these tests are still passing (which shouldn't be the problem, as I believe the time has passed for noncritical changes to 8.4), but it should definitely be merged first thing after 8.4 is released, if not before.
comment:53 followup: 54 Changed 4 years ago by
sorry for the delay, I was busy with the book itself.
Erik, the goal of including this into Sage is that people changing the behavior of Sage know when they break examples in the french book, which is the same I guess for other stuff in sage/tests
.
It would be nice if someone could review that ticket (as author of the book, I feel in conflict of interest).
comment:54 followup: 57 Changed 4 years ago by
Replying to zimmerma:
sorry for the delay, I was busy with the book itself.
Erik, the goal of including this into Sage is that people changing the behavior of Sage know when they break examples in the french book, which is the same I guess for other stuff in
sage/tests
.
I understand that obviously, and that wasn't my question. My question is to what extent do you expect Sage development to conform to the examples in the book and for how long; and what is your goal for handling cases where changes to Sage do necessarily break examples from the book? As we saw from moving from 8.3 to 8.4 there were changes that "broke" the examples, albeit usually in mostly trivial ways, but that were changes for the better that should not have been held back just because it would have caused some differences from published books.
IIUC the goal here is to just conform with the version of Sage that the book claims to conform to. But if the book needs to be updated in the future the question is how to go about that.
comment:55 followup: 56 Changed 4 years ago by
Maybe the book should avoid using ".next()", "<>", "xrange" and ".iteritems", no ??
See the patchbot plugins for the long list of python3 incompatibilities...
comment:56 followup: 58 Changed 4 years ago by
Replying to chapoton:
Maybe the book should avoid using ".next()", "<>", "xrange" and ".iteritems", no ??
See the patchbot plugins for the long list of python3 incompatibilities...
sorry, this is too late. The book is now in press. Maybe for the next edition?
comment:57 Changed 4 years ago by
Erik,
[...]
I understand that obviously, and that wasn't my question. My question is to what extent do you expect Sage development to conform to the examples in the book and for how long; and what is your goal for handling cases where changes to Sage do necessarily break examples from the book? As we saw from moving from 8.3 to 8.4 there were changes that "broke" the examples, albeit usually in mostly trivial ways, but that were changes for the better that should not have been held back just because it would have caused some differences from published books.
IIUC the goal here is to just conform with the version of Sage that the book claims to conform to. But if the book needs to be updated in the future the question is how to go about that.
the Sage developers will decide what is best, like they did in the past, between changing the behavior of Sage and breaking examples from published material. I cannot decide for them to what extent or how long.
The goal is (1) to inform the Sage developers about published material that they could break, and (2) to inform the authors of the book (by adding them in cc of the corresponding tickets) when changes are made that break some examples. In case (2) we will then discuss together casebycase, and maybe it will be an opportunity to improve the examples in the book.
comment:58 Changed 4 years ago by
Replying to zimmerma:
Replying to chapoton:
Maybe the book should avoid using ".next()", "<>", "xrange" and ".iteritems", no ??
See the patchbot plugins for the long list of python3 incompatibilities...
sorry, this is too late. The book is now in press. Maybe for the next edition?
Ok. But my point is that we are not going to accept here and now any doctest that does not pass on python3.
comment:59 followup: 62 Changed 4 years ago by
Cc:  Nicolas M. Thiéry added 

Ok. But my point is that we are not going to accept here and now any doctest that does not pass on python3.
Nicolas (which I added in cc) checked some time ago with Python3, and if I recall correctly, all tests did pass. Nicolas please can you confirm?
comment:60 Changed 4 years ago by
Many doctests are not passing with python3. Among them, some (a lot) are not your fault, just failing because sage is not yet fully ready to run with python3.
But many are caused by bad doctests in the books : use of .next, use of <>, use of xrange, etc
For example, every doctest starting with map must be wrapped with list(map(..)):
Failed example: map (cos, [0, pi/6, pi/4, pi/3, pi/2]) Expected: [1, 1/2*sqrt(3), 1/2*sqrt(2), 1/2, 0] Got: <map object at 0x7f1c2dd9cf28>
An example for xrange:
File "src/sage/tests/books/sagebook/integration_doctest.py", line 36, in sage.tests.books.sagebook.integration_doctest Failed example: interp_points = [(1+2*u/(n1), N(f(1+2*u/(n1)))) for u in xrange(n)] Exception raised: Traceback (most recent call last): File "/home/chapoton/sage3/local/lib/python3.6/sitepackages/sage/doctest/forker.py", line 659, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/chapoton/sage3/local/lib/python3.6/sitepackages/sage/doctest/forker.py", line 1070, in compile_and_execute exec(compiled, globs) File "<doctest sage.tests.books.sagebook.integration_doctest[7]>", line 2, in <module> for u in xrange(n)] NameError: name 'xrange' is not defined
And the global test result:
sage t long warnlong src/sage/tests/books/sagebook/combinat_doctest.py # 25 doctests failed sage t long warnlong src/sage/tests/books/sagebook/float_doctest.py # 3 doctests failed sage t long warnlong src/sage/tests/books/sagebook/graphique_doctest.py # 1 doctest failed sage t long warnlong src/sage/tests/books/sagebook/graphtheory_doctest.py # 11 doctests failed sage t long warnlong src/sage/tests/books/sagebook/lp_doctest.py # 1 doctest failed sage t long warnlong src/sage/tests/books/sagebook/mpoly_doctest.py # 19 doctests failed sage t long warnlong src/sage/tests/books/sagebook/programmation_doctest.py # 23 doctests failed sage t long warnlong src/sage/tests/books/sagebook/nonlinear_doctest.py # 24 doctests failed sage t long warnlong src/sage/tests/books/sagebook/linsolve_doctest.py # 2 doctests failed sage t long warnlong src/sage/tests/books/sagebook/polynomes_doctest.py # 1 doctest failed sage t long warnlong src/sage/tests/books/sagebook/integration_doctest.py # 5 doctests failed sage t long warnlong src/sage/tests/books/sagebook/sol/combinat_doctest.py # 1 doctest failed sage t long warnlong src/sage/tests/books/sagebook/sol/graphique_doctest.py # 2 doctests failed sage t long warnlong src/sage/tests/books/sagebook/sol/graphtheory_doctest.py # 1 doctest failed sage t long warnlong src/sage/tests/books/sagebook/sol/mpoly_doctest.py # 6 doctests failed sage t long warnlong src/sage/tests/books/sagebook/sol/nonlinear_doctest.py # 2 doctests failed sage t long warnlong src/sage/tests/books/sagebook/sol/numbertheory_doctest.py # 11 doctests failed sage t long warnlong src/sage/tests/books/sagebook/sol/polynomes_doctest.py # 4 doctests failed sage t long warnlong src/sage/tests/books/sagebook/sol/integration_doctest.py # 6 doctests failed
comment:61 Changed 4 years ago by
Cc:  vklein added 

comment:62 Changed 4 years ago by
Replying to zimmerma:
Ok. But my point is that we are not going to accept here and now any doctest that does not pass on python3.
Nicolas (which I added in cc) checked some time ago with Python3, and if I recall correctly, all tests did pass. Nicolas please can you confirm?
More precisely: I had checked with Python 2 together with from future imports (print_statements, division, absolute_import); this does not cover all Python3 incompatibilities.
comment:63 followup: 64 Changed 4 years ago by
Because we are now (painfully) trying to make all doctests pass with python3, this ticket will only be accepted with the necessary changes that make all or most of its doctests pass with python3. I am sorry that this forces some differences with the fresh new book.
On the other hand, I am surprised that this book for example still uses (and maybe even recommends using) <>
, which is known since along time to be deprecated.
comment:64 Changed 4 years ago by
Replying to chapoton:
Because we are now (painfully) trying to make all doctests pass with python3, this ticket will only be accepted with the necessary changes that make all or most of its doctests pass with python3. I am sorry that this forces some differences with the fresh new book.
this ticket would make no sense any more, if not checking the examples from the book.
We are in a dead end. I see no solution.
comment:65 followup: 67 Changed 4 years ago by
But couldn't you adapt the examples in the book to make them Python 3 compatible?
comment:66 Changed 4 years ago by
Milestone:  sage8.4 → sage8.5 

comment:67 Changed 4 years ago by
Replying to jdemeyer:
But couldn't you adapt the examples in the book to make them Python 3 compatible?
As Paul already demonstrated to me while working on this ticket earlier, the tex markup for the book is already such that it's possible to have multiple copies of a given example: One that is actually shown in the book, but excluded from the tests, and one that is hidden from the text but included in the generated doctests. So we could easily do that for the examples that are currently not Python 3 compatible.
Or, more simply, for now I could just modify the copies of the examples included in Sage to be Python 3 compatible. Most, if not all of these changes do not change the substance of the example, but rather are minor syntactic and spelling changes, mostly to do with Python itself (and not anything Sagespecific).
comment:68 Changed 4 years ago by
Branch:  u/embray/ticket23572 → public/french_book_python3 

Commit:  d8014819b4a93c41c5ef0cdd73a8851b11806a07 → b15724091409b162560c8eadfe9886b9b7ce24ee 
comment:69 Changed 4 years ago by
Commit:  b15724091409b162560c8eadfe9886b9b7ce24ee → 9bd7f8f5f3227b8d62e12c19443d84b88050dd7e 

Branch pushed to git repo; I updated commit sha1. New commits:
9bd7f8f  more py3 fixes

comment:70 Changed 4 years ago by
Commit:  9bd7f8f5f3227b8d62e12c19443d84b88050dd7e → 0a09136c26c89ac5f82c4bc28d77c74e80c5ab68 

Branch pushed to git repo; I updated commit sha1. New commits:
0a09136  even more py3 fixes

comment:71 Changed 4 years ago by
For the ones involving reduce
that you marked # py2
, instead you could import reduce
from functools
.
In fact, I always found it annoying that Guido took reduce
out of the builtins. I think for Sage on Python 3, in order to maintain compatibility, we should add reduce
back in as a default global.
Frédéric, could you add a new ticket to do that, and drop the # py2
stuff from this ticket and make this a dependency on the reduce
one? Then squash the the Python 3 fixes you added into a single commit (to make it a bit more manageable). After that's done we should set this positive_review.
As I suspected, none of the Python 3 fixes for these tests materially impact the tests in any semantically meaningful way.
comment:72 Changed 4 years ago by
Status:  needs_review → needs_work 

Work issues:  → support `reduce` as a global in Python 3 
comment:73 Changed 4 years ago by
Commit:  0a09136c26c89ac5f82c4bc28d77c74e80c5ab68 → ec5b502d1e7b20bdc3eedb11e9c4d5ed1eebfc9c 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ec5b502  python3 compatibility of doctests

comment:75 Changed 4 years ago by
Commit:  ec5b502d1e7b20bdc3eedb11e9c4d5ed1eebfc9c → faa2a2e5ab4fbb9ba3642285c03caec06402390c 

Branch pushed to git repo; I updated commit sha1. New commits:
faa2a2e  trac 23572 book: more py3 doctests fixed

comment:76 Changed 4 years ago by
Reviewers:  → Erik Bray 

Status:  needs_review → positive_review 
comment:77 Changed 4 years ago by
sorry, but I am not sure that the doctests do pass, as the patchbot has reported something strange. And they certainly do not all pass with python3..
comment:78 Changed 4 years ago by
Status:  positive_review → needs_work 

Well, please have a look then, because this looks related to some of the python 3 fixes you added (strangely). I also assumed when you wrote "done" you had squashed the python 3 fixeswhich you didbut it seems you've added more.
comment:79 Changed 4 years ago by
Owner:  changed from Erik Bray to Frédéric Chapoton 

comment:80 Changed 4 years ago by
Commit:  faa2a2e5ab4fbb9ba3642285c03caec06402390c → a4a22ae97f3ea60cf8e61478aecf8379f83be5c1 

Branch pushed to git repo; I updated commit sha1. New commits:
a4a22ae  trac 23572 fix doctest failing because book was moved

comment:81 Changed 4 years ago by
Status:  needs_work → needs_review 

comment:83 Changed 4 years ago by
Commit:  a4a22ae97f3ea60cf8e61478aecf8379f83be5c1 → 6d8f188bb44969dbdab282109070a8bf6f25667e 

Branch pushed to git repo; I updated commit sha1. New commits:
6d8f188  Merge branch 'public/french_book_python3' in 8.5.b3

comment:84 Changed 4 years ago by
Status:  needs_work → needs_review 

comment:85 Changed 4 years ago by
Why this change?

src/sage/tests/books/sagebook/combinat_doctest.py
diff git a/src/sage/tests/books/sagebook/combinat_doctest.py b/src/sage/tests/books/sagebook/combinat_doctest.py index 2192fe6..40a0aa1 100644
a b Sage example in ./combinat.tex, line 1895:: 590 590 Sage example in ./combinat.tex, line 1909:: 591 591 592 592 sage: cubes = [t**3 for t in range(999,1000)] 593 sage: exists([(x,y) for x in cubes for y in cubes], lambda x ,y: x+y== 218) # long593 sage: exists([(x,y) for x in cubes for y in cubes], lambda xy: sum(xy) == 218) # long 594 594 (True, (125, 343)) 595 sage: exists(((x,y) for x in cubes for y in cubes), lambda x ,y: x+y== 218) # long595 sage: exists(((x,y) for x in cubes for y in cubes), lambda xy: sum(xy) == 218) # long 596 596 (True, (125, 343)) 597 597 598 598 Sage example in ./combinat.tex, line 1927::
That doesn't look necessary...?
This one also looks like a more substantive change than it should be:

src/sage/tests/books/sagebook/graphtheory_doctest.py
diff git a/src/sage/tests/books/sagebook/graphtheory_doctest.py b/src/sage/tests/books/sagebook/graphtheory_doctest.py index c60c4a8..f64f416 100644
a b Sage example in ./graphtheory.tex, line 2070:: 360 360 361 361 Sage example in ./graphtheory.tex, line 2084:: 362 362 363 sage: G.independent_set() 364 [4, 6, 9, 11, 16, 21, 23, 26, 28, 33, 38, 43, 50, 365 56, 61, 71, 76, 78, 83, 88, 93, 95, 98, 100] 363 sage: X = G.independent_set(); G.is_independent_set(X) 364 True 366 365 367 366 Sage example in ./graphtheory.tex, line 2139::
If it happens that on Python 3 a different independent set is returned that's fine, but then we should mark that line # random
and add the G.is_independent_set(X)
test on a separate line.
I can fix the latter. But for the prior I wanted to check with you why the change was made since AFAICT there's no reason for it.
comment:86 Changed 4 years ago by
With python3:
sage: cubes = [t**3 for t in range(999,1000)] sage: exists([(x,y) for x in cubes for y in cubes], lambda xy: sum(xy) == 218) (True, (125, 343)) sage: exists([(x,y) for x in cubes for y in cubes], lambda x,y: x+y == 218)  TypeError Traceback (most recent call last) <ipythoninput3ea62578e4e4d> in <module>() > 1 exists([(x,y) for x in cubes for y in cubes], lambda x,y: x+y == Integer(218)) /home/chapoton/sage3/local/lib/python3.6/sitepackages/sage/misc/misc.py in exists(S, P) 1389 """ 1390 for x in S: > 1391 if P(x): 1392 return True, x 1393 return False, None TypeError: <lambda>() missing 1 required positional argument: 'y'
comment:87 Changed 4 years ago by
Commit:  6d8f188bb44969dbdab282109070a8bf6f25667e → b32170fe754f9293b8c7324414511c61f12ce64b 

comment:88 Changed 4 years ago by
The suggested change for independent set is done. The other suggestion is not right, see above.
comment:89 Changed 4 years ago by
Why did sage.misc.misc.exists
change/break? It looks like it used to be able to accept a list of tuples and now it can't?
comment:90 followup: 92 Changed 4 years ago by
nothing has changed, the "exists" failing doctests also fail with python2.
comment:92 followup: 93 Changed 4 years ago by
Replying to chapoton:
nothing has changed, the "exists" failing doctests also fail with python2.
I understand that's not a Python 3 specific thing, but something must have changed in Sage since the 8.4 release since that test didn't fail before...or did it? Was there some reason it wasn't being run?
comment:93 Changed 4 years ago by
Replying to embray:
Replying to chapoton:
nothing has changed, the "exists" failing doctests also fail with python2.
I understand that's not a Python 3 specific thing, but something must have changed in Sage since the 8.4 release since that test didn't fail before...or did it? Was there some reason it wasn't being run?
Well, AFAICT, it really hasn't changed at all recently. So I wonder how that test ever passed, unless it was never being run at all...
comment:94 Changed 4 years ago by
Ah, I see now. In the original test it used lambda (x,y):
, which actually gets parsed as tuple unpacking in the function signature (e.g. same as def f((x, y)):
) which was dropped in Python 3. That's very subtle since in the lambda
context it's less clear that that's what's going on (perhaps at least part of why it was dropped).
Okay.
comment:96 Changed 4 years ago by
So ? Can we move on ? Paul, what do you think ?
as I said in comment 64, this ticket has diverged from the book (now in print), thus I am no longer motivated to work on it.
comment:97 Changed 4 years ago by
Bonjour Paul.
Estce que ça veut dire qu'on peut valider ce ticket ? D'une certaine façon, les doctests sont ceux du livre, juste modifiés minimalement pour être valides sous python 3.
Je suis désolé que ça complique la relation avec le livre. C'est vraiment dommage que le livre ne soit pas compatible avec python 3.
comment:98 Changed 4 years ago by
yes please go on, after modifying the description to reflect the fact that these doctests are not exactly those from the book
comment:99 Changed 4 years ago by
Description:  modified (diff) 

I modified the ticket description. Needs review. Erik ?
one issue we encounter is the following. The code below differs between Sage 8.0 and Sage 8.1beta3:
With Sage 8.0 we get
0.0
, whereas with Sage 8.1beta3 we get0.0
. I thought the determinant code was deterministic, and since RDF computations follow IEEE 754, we should get a deterministic answer. How can we get different answers?