sagemath/sage

Introduce random-seed option to allow fuzzing of doctests

Closed this issue · 34 comments

kliem commented

This is the first step towards #29935.

We introduce an option for doctests: --random-seed.

This allows specifying which seed to use for tests
involving randomness.

The seed is displayed in the test log:

sage -t --long --random-seed=9876543210 src/sage/all.py 
...
Doctesting 1 file.
sage -t --long --random-seed=9876543210 src/sage/all.py
    [16 tests, 0.73 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.8 seconds
    cpu time: 0.7 seconds
    cumulative wall time: 0.7 seconds

which makes it easy to re-run tests with the same seed.

The seed defaults to 0 for now:

sage -t --long src/sage/all.py 
...
Doctesting 1 file.
sage -t --long --random-seed=0 src/sage/all.py
    [16 tests, 0.73 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.8 seconds
    cpu time: 0.7 seconds
    cumulative wall time: 0.7 seconds

but the plan in #29935 is to eventually have
the random seed itself picked at random by default.

CC: @slel

Component: doctest framework

Keywords: random

Author: Jonathan Kliem

Branch: 1d99129

Reviewer: Markus Wageringel, Matthias Koeppe

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

kliem commented

Branch: public/29962

kliem commented

New commits:

da1c6bestart from a "random" random seed for doctesting
b7b836dmake random seed reproducible
eedbe5edocument random_seed
998b1b9default random seed 0 for now
kliem commented

Commit: 998b1b9

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

23ed583fix doctest in hyperbolic_space/hyperbolic_point
5283dc4use abs tol flag
7b244c0modify doctests to the extend that they hold with fuzz
228f379Merge branch 'public/29936' of git://trac.sagemath.org/sage into public/29962
5c7e562fix double description of hypercube
e1bf211remove set_random_seed
0e7a998Merge branch 'public/29904' of git://trac.sagemath.org/sage into public/29962
b6a5dc7fix random test in geometry/linear_expression

Changed commit from 998b1b9 to b6a5dc7

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

Changed commit from b6a5dc7 to 998b1b9

Changed commit from 998b1b9 to 1d7b00e

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

1d7b00edash instead of underscore for command line options
kliem commented

Description changed:

--- 
+++ 
@@ -1,10 +1,10 @@
-We introduce an option for doctests `--random_seed` that defaults to `0` for now:
+We introduce an option for doctests `--random-seed` that defaults to `0` for now:
 
 ```
 sage -t --long src/sage/all.py 
 ...
 Doctesting 1 file.
-sage -t --long --random_seed=0 src/sage/all.py
+sage -t --long --random-seed=0 src/sage/all.py
     [16 tests, 0.73 s]
 ----------------------------------------------------------------------
 All tests passed!
comment:6

Overall, this looks good to me. The following changes seem to be needed:

-    sage: subprocess.call(["sage", "-t", "--warn-long", "0", "--random-seed=0", -random_seed.rst"], **kwds)  # long time
+    sage: subprocess.call(["sage", "-t", "--warn-long", "0", "--random-seed=0", "random_seed.rst"], **kwds)  # long time
-  - ``--random_seed[=seed]`` -- random seed for fuzzing doctests
+  - ``--random-seed[=seed]`` -- random seed for fuzzing doctests

Reviewer: Markus Wageringel

comment:7

The branch adds this to the documentation, which is not true for this ticket:

--- a/src/doc/en/developer/doctesting.rst
+++ b/src/doc/en/developer/doctesting.rst
...
+Doctests start from a random seed::
+
+    [kliem@sage sage-9.2]$ sage -t src/sage/doctest/tests/random_seed.rst
...
+    sage -t --warn-long 89.5 --random-seed=112986622569797306072457879734474628454 src/sage/doctest/tests/random_seed.rst
+    **********************************************************************
kliem commented

New commits:

b31e2d5Merge branch 'public/29962' of git://trac.sagemath.org/sage into public/29962-reb
2f30dd9small fixes
b62f781doctests do not start from a random seed by default yet
kliem commented

Changed commit from 1d7b00e to b62f781

kliem commented

Changed branch from public/29962 to public/29962-reb

Changed reviewer from Markus Wageringel to Markus Wageringel, Matthias Koeppe

comment:9

From my side this is a positive review - if patchbot is green

comment:10

Thanks. This successfully passes ptestlong.

kliem commented
comment:11

Thank you.

comment:12

Merge conflict

kliem commented

Changed commit from b62f781 to 1d99129

kliem commented
comment:13

sage -t --long --all passes for me.


New commits:

1d99129fix merge conflict
kliem commented

Changed branch from public/29962-reb to public/29962-reb2

kliem commented
comment:15

Thank you.

Changed branch from public/29962-reb2 to 1d99129

slel commented

Changed commit from 1d99129 to none

slel commented
comment:17

What random seeds are allowed? Any integer?
Of any sign? Arbitrarily large?

kliem commented
comment:18

This is how we will eventually get the random seeds.

sage: import sage.misc.randstate as randstate                                   
sage: randstate.set_random_seed()                                               
sage: randstate.initial_seed()                                                  
11032378495085541661748859066830408537

At least that's the plan for now. Any random seed that you can feed into set_random_seed should work.

(Edit: But I wouldn't be surprised if there are bugs somewhere with some strange random seed. There is no way, you can avoid all of them. But at least you can prepare for rather probable cases.)

slel commented
comment:19

The random_seed docstring says seed "must be coercible to a Python long".

Is that from -2**127 to 2**127 - 1?
Or are 32-bit and 64-bit systems different there?

Also isn't "long" a Python 2 concept,
with only "int" existing in Python 3?

Could we document the admissible range in a follow-up ticket, for non-experts like me?

slel commented
comment:20

And thanks for the work on varying random seeds when testing!

kliem commented
comment:21

I added this issue to #29935.

The idea was that once all parts of Sage are ready for it, the default would be to select a "random" random seed.

slel commented

Description changed:

--- 
+++ 
@@ -1,4 +1,7 @@
-We introduce an option for doctests `--random-seed` that defaults to `0` for now:
+Part of #29935.
+
+We introduce an option for doctests: `--random-seed`,
+that defaults to `0` for now:
 
 ```
 sage -t --long src/sage/all.py 
@@ -14,4 +17,4 @@
     cumulative wall time: 0.7 seconds
 ```
 
-This allows specifying, which random seed shall be used for the doctests.
+This allows specifying which random seed to use for the doctests.
slel commented

Description changed:

--- 
+++ 
@@ -1,7 +1,29 @@
-Part of #29935.
+This is the first step towards #29935.
 
-We introduce an option for doctests: `--random-seed`,
-that defaults to `0` for now:
+We introduce an option for doctests: `--random-seed`.
+
+This allows specifying which seed to use for tests
+involving randomness.
+
+The seed is displayed in the test log:
+
+```
+sage -t --long --random-seed=9876543210 src/sage/all.py 
+...
+Doctesting 1 file.
+sage -t --long --random-seed=9876543210 src/sage/all.py
+    [16 tests, 0.73 s]
+----------------------------------------------------------------------
+All tests passed!
+----------------------------------------------------------------------
+Total time for all tests: 0.8 seconds
+    cpu time: 0.7 seconds
+    cumulative wall time: 0.7 seconds
+```
+
+which makes it easy to re-run tests with the same seed.
+
+The seed defaults to `0` for now:
 
 ```
 sage -t --long src/sage/all.py 
@@ -17,4 +39,5 @@
     cumulative wall time: 0.7 seconds
 ```
 
-This allows specifying which random seed to use for the doctests.
+but the plan in #29935 is to eventually have
+the random seed itself picked at random by default.