optional doctest broken in oeis

[videlec]

Most of the doctests in oeis are broken (as the database has been updated).

To run the test do

./sage -t --optional=sage,internet SAGE_ROOT/src/sage/databases/oeis.py

Depends on #18637

CC: @sagetrac-tmonteil

Component: doctest coverage

Keywords: oeis

Author: Vincent Delecroix, Thierry Monteil

Branch/Commit: d457a51

Reviewer: Wilfried Luebbe, Frédéric Chapoton

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

[videlec]

Changed keywords from none to oeis

[videlec]

Description changed:

--- 
+++ 
@@ -1 +1,7 @@
 Most of the doctests in oeis are broken (as the database has been updated).
+
+To run the test do
+
+```
+sage -t --optional=sage,internet SAGE_ROOT/src/sage/database/oeis.py
+```

[videlec]

Commit: 1c3471f

[videlec]

Branch: u/vdelecroix/16252

[videlec]

New commits:

b570179	trac 14567: continued fraction (based on 6.2.rc0)
1c3471f	trac 16252: clean import + doctest errors in oeis

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

comment:2

2 doctest failures:

 ./sage -t --optional=sage,internet src/sage/databases/oeis.py
Running doctests with ID 2014-05-02-10-48-57-d24662df.
Doctesting 1 file.
sage -t src/sage/databases/oeis.py
**********************************************************************
File "src/sage/databases/oeis.py", line 835, in sage.databases.oeis.OEISSequence.keywords
Failed example:
    f.keywords()                          # optional -- internet
Expected:
    ('core', 'nonn', 'easy', 'nice', 'hear')
Got:
    ('core', 'nonn', 'easy', 'nice', 'hear', 'changed')
**********************************************************************
File "src/sage/databases/oeis.py", line 1730, in sage.databases.oeis.OEISSequence.programs
Failed example:
    ee = oeis('A001113') ; ee             # optional -- internet
Expected:
    A001113: Decimal expansion of e.
Got:
    Created new window in existing browser session.
    A001113: Decimal expansion of e.
**********************************************************************
2 items had failures:
   1 of   7 in sage.databases.oeis.OEISSequence.keywords
   1 of   7 in sage.databases.oeis.OEISSequence.programs
    [258 tests, 2 failures, 67.62 s]
----------------------------------------------------------------------
sage -t src/sage/databases/oeis.py  # 2 doctests failed

Have the keywords changed again ...,'nice', 'hear', 'changed')? Then one could consider this test as brittle :-/

And the statement Created new window in existing browser session. is true - while not expected. It opened http://oeis.org/A000045 a new tab in Chromium (ubuntu 14.04)

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

Description changed:

--- 
+++ 
@@ -3,5 +3,5 @@
 To run the test do

-sage -t --optional=sage,internet SAGE_ROOT/src/sage/database/oeis.py +./sage -t --optional=sage,internet SAGE_ROOT/src/sage/databases/oeis.py

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

458ad19

correct one doctest

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 1c3471f to 458ad19

[videlec]

comment:4

Hi Wilfried,

Thanks for looking at this.

I modified a bit the first test in order to make it more robust to small changes in the database. I do not understand what happened with the second one. Only the method browse and links should open some windows in your browser. Could you try again?

Vincent

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

comment:5

Hi Vincent,

the first failure is gone - the second not.

I replaced 45 by 10 in line 1645 (doctest of browse() and got

File "src/sage/databases/oeis.py", line 1645, in sage.databases.oeis.OEISSequence.browse
Failed example:
    f = oeis(10) ; f                      # optional -- internet, webbrowser
Expected:
    A000045: Fibonacci numbers: F(n) = F(n-1) + F(n-2) with F(0) = 0 and F(1) = 1.
Got:
    A000010: Euler totient function phi(n): count numbers <= n and prime to n.
**********************************************************************
File "src/sage/databases/oeis.py", line 1735, in sage.databases.oeis.OEISSequence.programs
Failed example:
    ee = oeis('A001113') ; ee             # optional -- internet
Expected:
    A001113: Decimal expansion of e.
Got:
    Created new window in existing browser session.
    A001113: Decimal expansion of e.
**********************************************************************
2 items had failures:
   1 of   3 in sage.databases.oeis.OEISSequence.browse
   1 of   7 in sage.databases.oeis.OEISSequence.programs
    [261 tests, 2 failures, 71.93 s]

Now the first failure is expected. My browser opened http://oeis.org/A000010. This seems to indicate that the new message Created new ... is left over by the doctest of browse(). But why? Don't you see this behavior?

This is what I get in a terminal

sage: import sage.databases.o
sage.databases.odlyzko  sage.databases.oeis     
sage: import sage.databases.oeis
sage: f = oeis(1)
sage: f
A000001: Number of groups of order n.
sage: f.browse()
sage: Created new window in existing browser session.

sage: f.show()
ID
A000001

After sage: Created new window in existing browser session. I had to press ENTER to see the sage: prompt again.

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

comment:6

The doctest failure is likely environment dependent:

The module webbrowser is a Python standard module. It use some logic to select a suitable browser and calls it with subprocess.Popen(). The message Created new ... is issued by Chromium (revealed be a Google search :-).

Today I closed my Chromium Version 34.0.1847.116 Ubuntu 14.04 aura (260972) before starting the test. The result:

File "src/sage/databases/oeis.py", line 1735, in sage.databases.oeis.OEISSequence.programs
Failed example:
    ee = oeis('A001113') ; ee             # optional -- internet
Expected:
    A001113: Decimal expansion of e.
Got:
    ATTENTION: default value of option force_s3tc_enable overridden by environment.
    [12960:12960:0505/151917:ERROR:sandbox_linux.cc(268)] InitializeSandbox() called with multiple threads in process gpu-process
    A001113: Decimal expansion of e.
**********************************************************************
1 item had failures:
   1 of   7 in sage.databases.oeis.OEISSequence.programs
    [261 tests, 1 failure, 78.67 s]

So there are environments (e.g. mine) that can produce some unexpected browser output.

At last I got All tests passed! with

            sage: ee = oeis('A001113') ; ee             # optional -- internet
            Created new window in existing browser session.
            A001113: Decimal expansion of e.

But this is not really a solution :-((

[videlec]

comment:7

Hi,

I do have a problem since the line

sage: ee = oeis('A001113')

does not call webbrowser. It is only the following that does

sage: ee.browse()

Hence I do not really understand what happens on your computer (even if it is now clear that it depends on the browser).

Maybe there is a way to turn off the writing of the subprocess on stdout/stderr... I will dig in. There are partial answers on stackoverflow but which are plateform and installation dependent... but it is better than nothing!

Vincent

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

comment:8

Hi Vincent,

the browser is called in a doctest of browse() in line 1645 sage: f = oeis(45) ; f.

This can be seen in my comment 5 where I changed oeis(45) to oeis(10). The browser opened A000010. The message from Chromium then surfaced in the other (later) doctest from line 1735.

Maybe this is (one more) hint that some test are better suited for a testing framework like nose or py.test. But that's a whole new story ...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 458ad19 to 6fa9592

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

6fa9592

trac #16252: typo that makes test failed!

[videlec]

comment:10

Hi,

There was a typo in the documentation. In order to skip the doctest one should write

sage: f.browse() # optional -- internet webbrowser

and not

sage: f.browse() # optional -- internet, webbrowser

I did the correction in the last commit. Could you try again? You should get no error with

./sage -t --optional=sage,internet SAGE_ROOT/src/sage/databases/oeis.py

but might get one with

./sage -t --optional=sage,internet,webbrowser SAGE_ROOT/src/sage/databases/oeis.py

Vincent

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

Reviewer: Wilfried Luebbe

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

comment:11

Hi,

just as you predicted ...

~/sage-6.2.rc0$ ./sage -t --optional=sage,internet src/sage/databases/oeis.py
Running doctests with ID 2014-05-05-21-13-21-9a2728e4.
Doctesting 1 file.
sage -t src/sage/databases/oeis.py
    [259 tests, 74.00 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 74.1 seconds
    cpu time: 1.4 seconds
    cumulative wall time: 74.0 seconds
wluebbe@Willis-Ubuntu:~/sage-6.2.rc0$ ./sage -t --optional=sage,internet,webbrowser src/sage/databases/oeis.py
Running doctests with ID 2014-05-05-21-16-48-fbdfca29.
Doctesting 1 file.
sage -t src/sage/databases/oeis.py
**********************************************************************
File "src/sage/databases/oeis.py", line 637, in sage.databases.oeis.OEISSequence.__init__
Failed example:
    sfibo = oeis('A039834')               # optional -- internet
Expected nothing
Got:
    Created new window in existing browser session.
**********************************************************************
File "src/sage/databases/oeis.py", line 1735, in sage.databases.oeis.OEISSequence.programs
Failed example:
    ee = oeis('A001113') ; ee             # optional -- internet
Expected:
    A001113: Decimal expansion of e.
Got:
    Created new window in existing browser session.
    Created new window in existing browser session.
    A001113: Decimal expansion of e.
**********************************************************************
2 items had failures:
   1 of   3 in sage.databases.oeis.OEISSequence.__init__
   1 of   7 in sage.databases.oeis.OEISSequence.programs
    [264 tests, 2 failures, 76.03 s]
----------------------------------------------------------------------
sage -t src/sage/databases/oeis.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 76.1 seconds
    cpu time: 1.4 seconds
    cumulative wall time: 76.0 seconds

Therefore I say: the patch looks good! - Doctest with (optional) webbrowser Chromium is a different story.

[videlec]

comment:12

Thanks!

Vincent

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:13

Hi,

i am a bit lost with the new framework, how it is possible that this ticket get positive review while #14567 is not settled ?

Could it be better to fix OEIS-changes in a self-contained ticket, and continued_fractions changes (even in oeis.py) in the other one ?

[videlec]

comment:14

Hi Thierry,

In the current state, this one will wait until #14567 gets reviewed. If there are some changes in #14567 then either there is a trivial merge and everything is fine, or the merge is non trivial and it will need another (short) review.

It might be better that #14567 gets reviewed... if you want to fix this one independently of #14567, it is up to you.

Vincent

[3a5ab40a-5e15-47f8-8025-1665d51c0660]

comment:16

While focusing on the doctest failures in oeis.py I missed the on dependency #14567 (and that the patch contained much more than oeis.py).

Therefore I think this ticket should stay with needs review until #14567 is positive. Then I will review this again :-)

Meanwhile I discovered some more optional doctest failures (#16302) ...

[videlec]

comment:20

The doctests are broken again due to updates in the sloane database...

Vincent

[edd8e884-f507-429a-b577-5d554626c0fe]

Changed branch from u/vdelecroix/16252 to u/tmonteil/16252

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:22

Since i could not merge Vincent's branch to the current develop branch, i made a new one based on latest 6.8.beta3 (and current version of OEIS).

---

New commits:

6818bfa	#16252 let oeis work with new continued fractions.
754a8cd	#16252 move rarely used import statements to methods.
8c68318	#16252 no space between doctest options.
c395b41	#16252 fix broken doctests.

[edd8e884-f507-429a-b577-5d554626c0fe]

Changed author from vincent delecroix to Vincent Delecroix, Thierry Monteil

[edd8e884-f507-429a-b577-5d554626c0fe]

Changed commit from 6fa9592 to c395b41

[jhpalmieri]

comment:23

I agree that the tests need to be fixed, but can we do it in such a way that they won't break frequently due to changes in the database?

[fchapoton]

Changed branch from u/tmonteil/16252 to public/ticket/16252

[fchapoton]

Changed commit from c395b41 to 80e0525

[fchapoton]

comment:24

I have made some pep8 cleanup.

I was about to give a positive review, but I agree it is not good to be forced to update the test all the time..

---

New commits:

80e0525

trac #16252 pep8 details

[jhpalmieri]

comment:25

To expand on this: because of #18558, many tests labeled "optional" will now be run. This is not true yet for "optional - internet" tests, but that could change, and then we want robust tests.

[jdemeyer]

comment:26

I don't like this: \xc3\xa9: can we please support unicode instead?

[jdemeyer]

Changed dependencies from #14567 to #18637

[jdemeyer]

comment:27

This needs to be rebased to #18637.

[jdemeyer]

comment:28

And this also looks like a regression:

- QQ as continued fractions
+ <class 'sage.rings.continued_fraction.ContinuedFraction_periodic'>

[videlec]

comment:29

Replying to @jdemeyer:

And this also looks like a regression:

- QQ as continued fractions
+ <class 'sage.rings.continued_fraction.ContinuedFraction_periodic'>

It is not (or at least independent from that ticket). Comes from #14567.

[jdemeyer]

comment:30

Replying to @videlec:

Comes from #14567.

I think it comes from this change on this ticket:

- return ContinuedFractionField()(self.first_terms())
+ from sage.rings.continued_fraction import continued_fraction
+ return continued_fraction(self.first_terms())

[videlec]

comment:31

Replying to @jdemeyer:

Replying to @videlec:

Comes from #14567.

I think it comes from this change on this ticket:

- return ContinuedFractionField()(self.first_terms())
+ from sage.rings.continued_fraction import continued_fraction
+ return continued_fraction(self.first_terms())

Right. But the continued fraction field is likely to be deprecated. It serves no purpose unless being there.

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:32

Concerning workaround doctests that are robust to database changes.

Replying to @jhpalmieri:

I agree that the tests need to be fixed, but can we do it in such a way that they won't break frequently due to changes in the database?

The "doc" part of the doctest is important too. As you can check, most failed doctests are designed to be examples for the user. I think the user should be exposed to what she will actually get, even if there is some small noise in the result, humans can adapt to such noise, and they need examples.

Replying to @fchapoton:

I was about to give a positive review, but I agree it is not good to be forced to update the test all the time..

We are not, the users are able to understand the examples even if they slightly differ from what they will get by testing themselves. In my opinion, those should be updated from time to time but there is no hurry to be on the edge, it is not like openssl or so.

Replying to @jhpalmieri:

To expand on this: because of #18558, many tests labeled "optional" will now be run. This is not true yet for "optional - internet" tests, but that could change, and then we want robust tests.

I am -1 to run the internet doctests by default, or we are going to flood OEIS servers for almost no benefit. Actually, this is why a fake OEIS entry is hardcoded in the file (those tests are run everytime).

The robustness can be achieved as follows : run internet doctests once before a release and fix them if they are severe (e.g. change in the database structure, change in the URL scheme).

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:33

Replying to @videlec:

Right. But the continued fraction field is likely to be deprecated. It serves no purpose unless being there.

To complement Vincent's answer, there is a semantic issue in using ContinuedFractionField().

The problem is that most (if not all) continued fractions in OEIS corrrespond to irrational numbers (OEIS is not much about periodic sequences), so a parent modelling the rationals is not appropriate, even if we only have the fist few partial coefficients (like elements of RDF are rationals but do not represent rational numbers). Perhaps should we use type instead of parent here (i used parent to be consistent with other cases) ?

Actually, i am planning to plug infinite sequences implemented in Sage with oeis calss (e.g. Fibonacci), so that we could ask for arbitrary many partial coefficients for the related continued fraction, not only those provided by .first_terms() method.

[jhpalmieri]

comment:34

Replying to @sagetrac-tmonteil:

Concerning workaround doctests that are robust to database changes.

Replying to @jhpalmieri:

I agree that the tests need to be fixed, but can we do it in such a way that they won't break frequently due to changes in the database?

The "doc" part of the doctest is important too.

Of course there has to be a balance, but I don't know of any examples where we intentionally allow failed doctests for the sake of this balance. Instead, it is often possible to write doctests which are both illustrative and correct.

Replying to @jhpalmieri:

To expand on this: because of #18558, many tests labeled "optional" will now be run. This is not true yet for "optional - internet" tests, but that could change, and then we want robust tests.

I am -1 to run the internet doctests by default, or we are going to flood OEIS servers for almost no benefit. Actually, this is why a fake OEIS entry is hardcoded in the file (those tests are run everytime).

I think there may be valid reasons to not run internet doctests by default, but I'm not convinced by this one. If you are worried that we might flood the OEIS servers, then rewrite the doctests to avoid this. For example, I can easily imagine someone adding "internet" to the optional doctest tags for the patchbots.

The robustness can be achieved as follows : run internet doctests once before a release and fix them if they are severe (e.g. change in the database structure, change in the URL scheme).

According to everything I know about Sage's doctesting, there is no tolerance for failed doctests. We don't fix them only if they are severe.

[jdemeyer]

comment:35

Replying to @videlec:

Right. But the continued fraction field is likely to be deprecated. It serves no purpose unless being there.

Fine, but then change the parent() doctest to type(). The fact that parent() returns type() for a non-Element is an implementation detail which shouldn't be exposed by these doctests.

(of course, I wonder why the result of continued_fraction() is no longer an Element but that's off topic)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

a7b0416

#16252 Add a `__getslice__` method to FancyTuple to fix comment 26.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 80e0525 to a7b0416

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from a7b0416 to 08557ad

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

08557ad

#16252 : parent -> type for continued fractions (comment 31)

[edd8e884-f507-429a-b577-5d554626c0fe]

comment:38

Replying to @jhpalmieri:

Of course there has to be a balance, but I don't know of any examples where we intentionally allow failed doctests for the sake of this balance. Instead, it is often possible to write doctests which are both illustrative and correct.

The goal of this ticket is precisely about fixing broken doctests. We do not intentionally allow failed doctests, they will all be illustrative and correct once this ticket get merged. I am just saying that such broken doctests do not exhibit new bugs or regression in Sage, but only minor changes in remote database. As such, they are less important than all the nonfixed bugs that are waiting on trac without breaking any doctest. The aim of this module is about querying a remote dynamic database, if we want to be illustrative, we should show the results provided by the database, and they are subject to changes.

I think there may be valid reasons to not run internet doctests by default, but I'm not convinced by this one. If you are worried that we might flood the OEIS servers, then rewrite the doctests to avoid this. For example, I can easily imagine someone adding "internet" to the optional doctest tags for the patchbots.

The thing is that i do not want to use something like not tested because it should be possible to test them in order to fix them easily. Since the doctests should still be runable, they will be runable by any patchbot as well. What do you suggest ? Should we add a please_do_not_run_by_default keyword ?

[jdemeyer]

comment:39

Replying to @sagetrac-tmonteil:

Should we add a please_do_not_run_by_default keyword ?

I think that keyword is called optional - internet.