sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.09k stars 394 forks source link

doctests for the english translation of the book "Calcul mathématique avec Sage" #23572

Closed zimmermann6 closed 5 years ago

zimmermann6 commented 6 years ago

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:

  1. Using scipy.sparse.lil_matrix is now broken: #23867

Depends on #27066

Upstream: Reported upstream. No feedback yet.

CC: @mforets @dcoudert @nthiery @vinklein @jhpalmieri

Component: doctest coverage

Author: Erik Bray, Frédéric Chapoton, Jeroen Demeyer

Branch/Commit: ae36049

Reviewer: Erik Bray, Frédéric Chapoton, Vincent Klein

Issue created by migration from https://trac.sagemath.org/ticket/23572

zimmermann6 commented 6 years ago
comment:2

one issue we encounter is the following. The code below differs between Sage 8.0 and Sage 8.1beta3:

   def NearlySingularMatrix(R,n):
      M=matrix(R,n,n)
      for i in range(0,n):
         for j in range(0,n):
            M[i,j]= (1+log(R(1+i)))/((i+1)^2+(j+1)^2)
      return M
   n=35
   print NearlySingularMatrix(RDF,n).det()

With Sage 8.0 we get 0.0, whereas with Sage 8.1beta3 we get -0.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?

zimmermann6 commented 6 years ago
comment:3

another issue:

┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 8.0, Release Date: 2017-07-21                     │
│ Type "notebook()" for the browser-based 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?

zimmermann6 commented 6 years ago
comment:4

for comment [comment:3], I'll ask David Coudert who worked on edge_coloring in #22564.

Paul

dcoudert commented 6 years ago
comment:6

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).

zimmermann6 commented 6 years ago
comment:7

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

dcoudert commented 6 years ago
comment:8

Replying to @zimmermann6:

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:

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.

zimmermann6 commented 6 years ago
comment:9

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.

jdemeyer commented 6 years ago

Replying to @zimmermann6:

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...

zimmermann6 commented 6 years ago
comment:11

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.

zimmermann6 commented 6 years ago
comment:12

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(i-1)]=-h2
  ....:    if i+n<n2: A[i,int(i+n)]=-h2
  ....:    if i-n>=0: A[i,int(i-n)]=-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 [comment:6] above). If that does not work, is there a random seed that we can set to make those commands deterministic?

jdemeyer commented 6 years ago
comment:13

Replying to @zimmermann6:

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 [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 non-deterministic or just different with different versions of Sage?

jdemeyer commented 6 years ago
comment:14

Replying to @zimmermann6:

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

This is a good and intentional change: just change the expected output.

jdemeyer commented 6 years ago
comment:15

Replying to @zimmermann6:

Aren't lil_matrix tested in the current doctests?

lil_matrix is not used or tested anywhere in Sage. I'll have a look.

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 6 years ago
comment:16

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'. 
jdemeyer commented 6 years ago

Upstream: Reported upstream. No feedback yet.

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -4,4 +4,5 @@

 We should probably wait for the final version of the book before adding/updating doctests.

-
+**Upstream bugs**:
+1. Using `scipy.sparse.lil_matrix` is now broken: https://github.com/numpy/numpy/pull/9691
zimmermann6 commented 6 years ago
comment:18

Replying to @jdemeyer:

Replying to @zimmermann6:

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 [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 non-deterministic or just different with different versions of Sage?

no idea, I first asked Nicolas to test with solver='GLPK'.

zimmermann6 commented 6 years ago
comment:19

Replying to @jdemeyer:

Replying to @zimmermann6:

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

This 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.

jdemeyer commented 6 years ago
comment:20

Replying to @zimmermann6:

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.

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -5,4 +5,4 @@
 We should probably wait for the final version of the book before adding/updating doctests.

 **Upstream bugs**:
-1. Using `scipy.sparse.lil_matrix` is now broken: https://github.com/numpy/numpy/pull/9691
+1. Using `scipy.sparse.lil_matrix` is now broken: https://github.com/scipy/scipy/issues/7860 and https://github.com/numpy/numpy/pull/9691
jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -4,5 +4,5 @@

 We should probably wait for the final version of the book before adding/updating doctests.

-**Upstream bugs**:
-1. Using `scipy.sparse.lil_matrix` is now broken: https://github.com/scipy/scipy/issues/7860 and https://github.com/numpy/numpy/pull/9691
+**bugs found**:
+1. Using `scipy.sparse.lil_matrix` is now broken: #23867
jdemeyer commented 6 years ago
comment:23

I opened #23867 for the lil_matrix() issue.

jdemeyer commented 6 years ago
comment:24

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'>.

zimmermann6 commented 6 years ago
comment:25

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.

jdemeyer commented 6 years ago
comment:26

For Sage doctests, you should write <... 'int'> instead of <type 'int'>.

zimmermann6 commented 6 years ago
comment:27

good idea! I will do that change in the doctests.

embray commented 5 years ago
comment:28

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.

embray commented 5 years ago
comment:29

I can take this over.

embray commented 5 years ago
comment:30

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.

embray commented 5 years ago

Branch: u/embray/ticket-23572

embray commented 5 years ago

Commit: 7f3490e

embray commented 5 years ago
comment:31

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.e-8)
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:

7f3490ereplace sage/tests/french_book with sage/tests/books/sagebook, containing
embray commented 5 years ago

Author: Erik Bray

zimmermann6 commented 5 years ago
comment:33

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.e-8, 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?

embray commented 5 years ago
comment:34

I'm not sure what to do about this--I 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.

embray commented 5 years ago
comment:35

Either way it would require manually modifying the test module.

zimmermann6 commented 5 years ago
comment:36

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.

embray commented 5 years ago
comment:37

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 as-is, 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.

zimmermann6 commented 5 years ago
comment:38

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 as-is, 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).

embray commented 5 years ago
comment:39

Perhaps I'll just create a branch on the book repo for now to add any relevant updates.

embray commented 5 years ago
comment:40

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 1e-16
Expected:
    73.62914262423378365
Got:
    73.629142624233783843668417691077783339
Tolerance exceeded:
    73.62914262423378365 vs 73.629142624233783843668417691077783339, tolerance 2e-16 > 1e-16
**********************************************************************
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 1e-16
Expected:
    74.47274932028288503
Got:
    74.472749320282885192304428135608736736
Tolerance exceeded:
    74.47274932028288503 vs 74.472749320282885192304428135608736736, tolerance 2e-16 > 1e-16
**********************************************************************
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 1e-15
Expected:
    0.250000000000001
Got:
    0.249999999999999
Tolerance exceeded:
    0.250000000000001 vs 0.249999999999999, tolerance 2e-15 > 1e-15
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.

zimmermann6 commented 5 years ago
comment:41

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?

embray commented 5 years ago
comment:42

Replying to @zimmermann6:

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 re-copied 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...?

zimmermann6 commented 5 years ago
comment:43

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.

f588ca1e-f96a-4dea-804d-c55eab873dab commented 5 years ago
comment:44

Replying to @zimmermann6:

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).

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...

embray commented 5 years ago
comment:45

Sorry, the above was me. Forgot I was still logged in as my bot :)

embray commented 5 years ago
comment:46

Okay, I double-checked 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:

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/src/sage/tests/books/sagebook/integration_doctest.py
+++ b/src/sage/tests/books/sagebook/integration_doctest.py
@@ -106,7 +106,7 @@ Sage example in ./integration.tex, line 717::
 Sage example in ./integration.tex, line 745::

   sage: p = gp.set_precision(old_prec) # we reset the default precision
-  sage: gp('intnum(x=0, 1, x^(-99/100))') # abs tol 1e-16
+  sage: gp('intnum(x=0, 1, x^(-99/100))') # abs tol 2e-16
   73.62914262423378365

 Sage example in ./integration.tex, line 753::
@@ -116,7 +116,7 @@ Sage example in ./integration.tex, line 753::

 Sage example in ./integration.tex, line 764::

-  sage: gp('intnum(x=[0, -1/42], 1, x^(-99/100))') # abs tol 1e-16
+  sage: gp('intnum(x=[0, -1/42], 1, x^(-99/100))') # abs tol 2e-16
   74.47274932028288503

 Sage example in ./integration.tex, line 783::
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/src/sage/tests/books/sagebook/linsolve_doctest.py
+++ b/src/sage/tests/books/sagebook/linsolve_doctest.py
@@ -382,7 +382,7 @@ Sage example in ./linsolve.tex, line 2721::
   ....:     m[i,i] = 1./A[i,i]
   sage: msc = m.tocsc()
   sage: from scipy.sparse.linalg import cg
-  sage: x = cg(A, b, M = msc, tol=1.e-8)
+  sage: x = cg(A, b, M=msc, atol=1.e-8)

 Sage example in ./linsolve.tex, line 2782::

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/src/sage/tests/books/sagebook/numbertheory_doctest.py
+++ b/src/sage/tests/books/sagebook/numbertheory_doctest.py
@@ -34,7 +34,7 @@ Sage example in ./numbertheory.tex, line 174::
   sage: 1/a
   Traceback (most recent call last):
     ...
-  ZeroDivisionError: Inverse does not exist.
+  ZeroDivisionError: inverse of Mod(3, 15) does not exist

 Sage example in ./numbertheory.tex, line 199::

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/src/sage/tests/books/sagebook/polynomes_doctest.py
+++ b/src/sage/tests/books/sagebook/polynomes_doctest.py
@@ -238,7 +238,7 @@ Sage example in ./polynomes.tex, line 1399::
 Sage example in ./polynomes.tex, line 1411::

   sage: r.reduce(); r
-  1.00000000000000/(-x + 1.00000000000000)
+  -1.00000000000000/(x - 1.00000000000000)

 Sage example in ./polynomes.tex, line 1487::

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/src/sage/tests/books/sagebook/sol/integration_doctest.py
+++ b/src/sage/tests/books/sagebook/sol/integration_doctest.py
@@ -36,7 +36,7 @@ Sage example in ./sol/integration.tex, line 58::

   sage: numerical_integral(x * log(1+x), 0, 1)
   (0.25, 2.7755575615628914e-15)
-  sage: N(QuadNC(lambda x: x * log(1+x), 0, 1, 19)) # abs tol 1e-15
+  sage: N(QuadNC(lambda x: x * log(1+x), 0, 1, 19)) # abs tol 2e-15
   0.250000000000001
   sage: numerical_integral(sqrt(1-x^2), 0, 1)
   (0.785398167726482..., 9.042725224567119...e-07)

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.

zimmermann6 commented 5 years ago
comment:47

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?

embray commented 5 years ago
comment:48

Replying to @zimmermann6:

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 double-checked.

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.

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.

embray commented 5 years ago
comment:49

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.