implicitly fuzz RNG-dependent doctests with a random random seed
Closed this issue · 249 comments
Our documentation and tests include a variety of
examples and tests involving randomness.
Those depend on a randomness seed, and up to Sage 9.1
they always used the same one: 0. Thus every run
of sage -t or make [p]test[all][long] would run
those "random" doctests deterministically.
In many cases, the output of those tests even relied
on that. As a result, random examples and tests
were actually testing that they were run non-randomly!
This is reminiscent of
an xkcd comic illustration of random number generators.
Based on these considerations and the
related sage-devel discussion,
we propose to:
- allow specifying a randomness when running tests (#29962),
- adapt tests involving randomness, making sure they
test mathematical functionality independent of
the randomness seed used to run them (see roadmap), - default to a random randomness seed when none is specified
(present ticket),
thus making those tests more robust and more useful,
by becoming more likely to reveal bugs in a variety
of cases including corner cases.
The first step (see #29962, merged in Sage 9.2)
adds a --random-seed flag to sage -t, allowing:
sage -t --long --random-seed=9876543210 src/sage/
Still, when no randomness seed is specified,
the default seed 0 is used. This means most testers
test with the same randomness seed, making "random"
doctests still mostly deterministic in practice.
Here is a way to pick a random randomness seed
and run tests with it (can be used to work on
the tickets in the roadmap):
$ randseed() {
sage -c "import sage.misc.randstate as randstate; \
randstate.set_random_seed(); \
print(randstate.initial_seed())";
}
$ SEED=$(randseed)
$ DIR=src/sage
$ echo "$ sage -t --long --random-seed=${SEED} ${DIR}" \
&& ./sage -t --long --random-seed=${SEED} ${DIR}
Once examples and tests involving randomness
have been adapted, the present ticket puts
the final touch by making it so that
when running tests with no seed specified,
a random one will be used:
sage -t src/sage/all.py
Running doctests with ID 2020-06-23-23-19-03-8003eea5.
...
sage -t --warn-long 89.5 --random-seed=273987373473433732996760115183658447263 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
Being displayed in the output, the seed used
can be used again if needed:
sage -t --warn-long 89.5 --random-seed=273987373473433732996760115183658447263 src/sage/all.py
allowing to explore any problematic case
revealed by running the tests with that seed.
Roadmap:
- Allow fuzzing: #29962
- Make all parts of sage ready for default fuzzing:
- #29945: coding
- #29963: geometry
- #29964: libs
- #29965: graphs
- #32107: groups
- #29967: interfaces
- #29968: algebras
- #29969: misc
- #29970: arith
- #29971: categories
- #29972: stats
- #29973: sets
- #29974: combinat
- #29975: numerical, probability
- #29976: matrix
- #29977: modular
- #29978: modules
- #29979: rings
- #29980: crypto
- #29981: documentation
- #29982: dynamics
- #29983: finance
- #29984: symbolic
- #29985: schemes
- #29986: plot
- #32188: various missed tests along the way
- #32216: Update the developers guide for implicitly fuzzing doctests
- Finally make fuzzing default with this ticket.
Follow-up:
- #32544: Meta-ticket: Fix unstable doctests detected after #29935
- Remove
set_random_seedin doctests where possible.
Errors discovered by this ticket:
- #29936: Hyperbolic geometry bugs revealed by fuzzing
- #29945: Failing doctest in
src/sage/coding/linear_code.py
(just a wrong doctest, will be fixed here) - #29954: Unstable plotting
- #29956: Bug in
KlyachkoBundle_class.random_deformation - #29957: Bug in
ContinuedFractionrounding - #29958: Too many strong articulation points
- #29961: Random symbolic expression is completely unstable
- #30045: Bug in Reed-Solomon encoder and error-erasure decoder
- Index error with random derangement (fixed in #29974)
- #31890: simplify_hypergeometric is unstable
- #31891:
ZeroDivisonErrorwhen creating polynomial system - #31892: Conic parametrization broken
- #32075: Polynomial generic power trunk broken
- #32083: Various errors with polybori including segmentation fault
- #32084
_nth_root_naivefails for integer mod - #32085: Errors when computing norms of padic elements
- #32086: apply_homography unstable for continue fraction
- #32095: DiFUB algorithm fails on some random graph
- #32108: Fix random tree on one or less vertices
- #32109: Fix 0/0 in ore function field
- #32111: Unstable minimal polynomial for element of 2-adic Eisenstein Extension Field in pi defined by x^4 - 2*a
- #32117: Random relative number field checks only irreducibility over QQ
- #32118: AlgebraicForm checks invariance with random matrix that can be the identity
- #32124: SL2Z.random_element unstable
- #32125: random Ore polynomials do not respect minimum degree bound
- #32126: padic QpLC.random_element is broken
- #32127: gosper_iterator of continued fractions is unstable
- #32129: sage_input is unreliable for elements of ComplexField
- #32131: Cut width of graph with one edge incorrect
- #32132: Wrong gyration orbit length
- #32138: is_groebner fails over fraction fields
- #32141: Unstable doctest involving permutation groups
- #32169: Bug in edge disjoint spanning trees
- #32185: Failing weak order assertion on random symbolic expression
- #32186: Random bounded tolerance graph
- #32187: permutation group generated by list perms in L of degree n incorrect when compared to GAP
- #32657:
plot_vector_fieldunstable
Depends on #32667
CC: @kliem @orlitzky @DaveWitteMorris @slel @mantepse
Component: doctest framework
Keywords: fuzz, random, seed
Author: Jonathan Kliem
Branch/Commit: 047379c
Reviewer: Michael Orlitzky
Issue created by migration from https://trac.sagemath.org/ticket/29935
Description changed:
---
+++
@@ -5,3 +5,21 @@
See this thread: https://groups.google.com/d/msg/sage-devel/c4UbKSdt3Aw/UQAo1iYoAAAJ
for more details.
+
+The doctest framework modification is trivial:
+
+```diff
+--- a/src/sage/doctest/forker.py
++++ b/src/sage/doctest/forker.py
+@@ -848,7 +848,7 @@ class SageDocTestRunner(doctest.DocTestRunner, object):
+ TestResults(failed=0, attempted=4)
+ """
+ self.setters = {}
+- randstate.set_random_seed(0)
++ randstate.set_random_seed()
+ # scipy 1.18 introduced reprecation warnings on a number of things they are moving to
+ # numpy, e.g. DeprecationWarning: scipy.array is deprecated
+ # and will be removed in SciPy 2.0.0, use numpy.array instead
+```
+
+but many doctests need to be fixed.Just so that we don't forget that we will have to update
https://doc.sagemath.org/html/en/developer/coding_basics.html
Work Issues: modify coding conventions
geometry/hyperbolic_space/hyperbolic_geodesic.py makes some claims on how good approximations work, which doesn't seem to work. I had this thing run 7 times now and not one time did all tests pass. E.g. there is one test that gives me absolute errors of 0.7 when running this in the shell, but the test claims it is below 10**-9.
There is also #29904. Other than that the geometry module appears to ready.
Edit: And there is some place where you need to add set_random_seed(0) now or similar.
Replying to @kliem:
geometry/hyperbolic_space/hyperbolic_geodesic.pymakes some claims on how good approximations work, which doesn't seem to work. I had this thing run 7 times now and not one time did all tests pass. E.g. there is one test that gives me absolute errors of0.7when running this in the shell, but the test claims it is below10**-9.
this is a great example of fuzzing catching an apparent error, it seems.
There is also #29904. Other than that the
geometrymodule appears to ready.Edit: And there is some place where you need to add
set_random_seed(0)now or similar.
Branch: public/29935
Even in the developer guide there is an example, where it is confusing:
sage: M = matrix.identity(3) + random_matrix(RR,3,3)/10^3
sage: M^2 # abs tol 1e-2
[1 0 0]
[0 1 0]
[0 0 1]
There is no mentioning there that only one particular matrix is tested.
Branch pushed to git repo; I updated commit sha1. New commits:
dc49dd0 | update developer guide |
Changed work issues from modify coding conventions to none
Replying to @kliem:
Even in the developer guide there is an example, where it is confusing:
sage: M = matrix.identity(3) + random_matrix(RR,3,3)/10^3 sage: M^2 # abs tol 1e-2 [1 0 0] [0 1 0] [0 0 1]There is no mentioning there that only one particular matrix is tested.
indeed it assumes (?) that the entries of the random matrix are small in abs value
Nothing to assume there. random_matrix(RR,3,3) has entries between -1 and 1. After dividing we are good.
How about making the random seed an option to sage-runtests. Then people can fuzz the doctests if they want.
Will people do that? Especially enough to find incorrect claims. If you look into #29936 we have been claiming a preciseness that is far from true. Even if you consider the relative error instead of the absolute error. fuzzed doctests would have easily detected that. Even worse. Maybe the situation was better at some point and we never caught on about the regression.
it is not a "real" (time-consuming) fuzzing, it is making the main random seed nondeterministic in tests. making it optional is akin to making tests optional.
we already found a couple of examples which show usefulness of this change in detecting bugs in Sage.
The option that I propose to add needs to be added in any case, so that when a failure is revealed by a random random seeds, we can reproduce the failure with that seed.
it is a good point - and the non-det. seed needs to be logged - if this is not yet in the branch it should get there.
So I would suggest to use this ticket to introduce the option, not change the default.
no, I don't agree, it makes no sense to test less if one can test more, and also not having it default would not force people to make their tests robust.
By the way I don't think most doctests have the ambition to demonstrate correctness of mathematical claims. In my opinion, if a doctest intends to do that, it should run an explicit loop of several tests. Still deterministically.
Why would creating the option and changing the default have to be done on the same ticket? I think it's best practices to separate the two steps on two tickets.
Feel free to do with the branch whatever you think is reasonable.
What I think would be great if running we could run each file at some "random" seed, e.g.
sage -t src/sage/all.py
Running doctests with ID 2020-06-22-18-56-54-668a26f3.
Git branch: public/29935
Using --optional=4ti2,bliss,build,dochtml,e_antic,flask,flask_autoindex,flask_babel,flask_oldsessions,flask_openid,flask_silk,latte_int,lidia,lrslib,memlimit,normaliz,pynormaliz,python_openid,sage,speaklater,twisted
Doctesting 1 file.
sage -t --warn-long 91.5 --seed 123456 src/sage/all.py
[16 tests, 0.76 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.8 seconds
cpu time: 0.8 seconds
cumulative wall time: 0.8 seconds
You can reproduce this test with
sage -t --warn-long 91.5 --seed 123456 src/sage/all.py
Is a bit annoying, as seeds can be long though. Then whenever a bot or someone discovers a failure, you can reproduce it. At least if it really was caused by the random seed.
Just to weigh in:
- The default should be to use a randomly-chosen seed.
- Failures should be reproducible:
a. It should be possible to pass a seed to the test runner with e.g.--seed.
b. Any test failures should log/echo the current seed so that the failing test can be reproduced easily using the option in (2.a.).
The sooner we can push this through the better. Tests that don't work due to bugs can even be deleted and replaced with trac tickets saying "this never worked" IMO.
With #29904 (already pulled into this branch), #29936 (not yet pulled) and b2954ceea638b4510e99d90cea3f99fa9a49338 (commit from above) geometry should be ready for this.
Edit: The updated branch of #29904 (not yet pulled) doesn't have set_random_seed anymore. But there is plenty of set_random_seed for cones doctests? Should I remove those as well or do we do this somewhere else?
Branch pushed to git repo; I updated commit sha1. New commits:
bac708e | make graphs ready for fuzz doctests |
Replying to @kliem:
there is plenty of
set_random_seedfor cones doctests? Should I remove those as well or do we do this somewhere else?
Not on the same ticket.
Branch pushed to git repo; I updated commit sha1. New commits:
b5e9896 | make algebras fuzz ready |
Branch pushed to git repo; I updated commit sha1. New commits:
8babc2a | make arith fuzz ready |
Please tell me, if I should open seperate tickets for those or something else. I just felt like starting with those. I'm done for today.
Replying to @orlitzky:
- The default should be to use a randomly-chosen seed.
- Failures should be reproducible:
a. It should be possible to pass a seed to the test runner with e.g.--seed.
b. Any test failures should log/echo the current seed so that the failing test can be reproduced easily using the option in (2.a.).
+1.
Also note that Sage still uses a verbatim copy of the Python-2 version of the Random module during doctesting, which was necessary in order to obtain consistent results between Python 2 and 3. This is why the bug in #29550 went unnoticed, for example. Now that support for Python 2 has been removed, we should switch to the Python 3 version of Random. This will require updating lots of doctests, but I assume this ticket here will simplify this if it makes doctests less dependent on the seed/RNG.
Description changed:
---
+++
@@ -6,20 +6,28 @@
See this thread: https://groups.google.com/d/msg/sage-devel/c4UbKSdt3Aw/UQAo1iYoAAAJ
for more details.
-The doctest framework modification is trivial:
+If not set, a random seed will be chosen for doctesting:
-```diff
---- a/src/sage/doctest/forker.py
-+++ b/src/sage/doctest/forker.py
-@@ -848,7 +848,7 @@ class SageDocTestRunner(doctest.DocTestRunner, object):
- TestResults(failed=0, attempted=4)
- """
- self.setters = {}
-- randstate.set_random_seed(0)
-+ randstate.set_random_seed()
- # scipy 1.18 introduced reprecation warnings on a number of things they are moving to
- # numpy, e.g. DeprecationWarning: scipy.array is deprecated
- # and will be removed in SciPy 2.0.0, use numpy.array instead
+```
+sage -t src/sage/all.py
+Running doctests with ID 2020-06-23-23-19-03-8003eea5.
+...
+sage -t --warn-long 89.5 --random_seed=273987373473433732996760115183658447263 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 many doctests need to be fixed.
+This can then be reproduced with
+
+```
+sage -t --warn-long 89.5 --random_seed=273987373473433732996760115183658447263 src/sage/all.py
+```
+
+Errors discovered by this ticket:
+- #29939: Fix precicion for hyperbolic geodesic
+- #29945: Failing doctest in src/sage/coding/linear_code.py I specified random_seed to distinguish from --randorder=SEED. If after --randorder=SEED the next option is --seed=SEED that sounds a bit weird to me. Although random_seed is not much better.
Standard bracked lyndon words are set up incorrectly. I don't know if that should be consider a bug:
sage: SBLW33 = StandardBracketedLyndonWords(3,3)
sage: TestSuite(SBLW33).run()
Failure in _test_an_element:
Traceback (most recent call last):
File "/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/misc/sage_unittest.py", line 296, in run
test_method(tester=tester)
File "/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/categories/sets_cat.py", line 1093, in _test_an_element
tester.assertTrue(an_element in self, "self.an_element() is not in self")
File "/home/jonathan/Applications/sage/local/lib/python3.7/unittest/case.py", line 692, in assertTrue
raise self.failureException(msg)
AssertionError: self.an_element() is not in self
------------------------------------------------------------
Failure in _test_enumerated_set_contains:
Traceback (most recent call last):
File "/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/misc/sage_unittest.py", line 296, in run
test_method(tester=tester)
File "/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/categories/enumerated_sets.py", line 945, in _test_enumerated_set_contains
tester.assertIn(w, self)
File "/home/jonathan/Applications/sage/local/lib/python3.7/unittest/case.py", line 1106, in assertIn
self.fail(self._formatMessage(msg, standardMsg))
File "/home/jonathan/Applications/sage/local/lib/python3.7/unittest/case.py", line 680, in fail
raise self.failureException(msg)
AssertionError: [1, [1, 2]] not found in Standard bracketed Lyndon words from an alphabet of size 3 of length 3
------------------------------------------------------------
Failure in _test_some_elements:
Traceback (most recent call last):
File "/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/misc/sage_unittest.py", line 296, in run
test_method(tester=tester)
File "/home/jonathan/Applications/sage/local/lib/python3.7/site-packages/sage/categories/sets_cat.py", line 1385, in _test_some_elements
"the object %s in self.some_elements() is not in self")%(x,))
File "/home/jonathan/Applications/sage/local/lib/python3.7/unittest/case.py", line 692, in assertTrue
raise self.failureException(msg)
AssertionError: the object [1, [1, 2]] in self.some_elements() is not in self
------------------------------------------------------------
The following tests failed: _test_an_element, _test_enumerated_set_contains, _test_some_elements
Branch pushed to git repo; I updated commit sha1. New commits:
93415f3 | make combinat fuzz ready |
Branch pushed to git repo; I updated commit sha1. New commits:
dbd7f6c | fix doctest in sage/coding/linear_code |
Description changed:
---
+++
@@ -30,4 +30,5 @@
Errors discovered by this ticket:
- #29939: Fix precicion for hyperbolic geodesic
-- #29945: Failing doctest in src/sage/coding/linear_code.py
+- #29945: Failing doctest in src/sage/coding/linear_code.py (just a wrong doctest, will be fixed here)
+- #29954: Unstable plottingIn src/sage/plot/multigraphics.py is a doctest that fails with the precicion:
1294 sage: g1 = plot(sin(x), (x, -pi, pi))
1295 sage: g2 = circle((0,1), 1.)
1296 sage: G = graphics_array([g1, g2])
1297 sage: G.position(0) # tol 1.0e-13
1298 (0.023437500000000003,
1299 0.03415488992713045,
1300 0.4627803348994754,
1301 0.9345951100728696)
1302 sage: G.position(1) # tol 1.0e-13
1303 (0.5136230468749999,
1304 0.19293222169724827,
1305 0.46278033489947534,
1306 0.617040446532634)
when running on set_random_seed(151058820726654196682836430928254760259). The precision is somewhere around 1e-3 then. Is this a bug?
Description changed:
---
+++
@@ -32,3 +32,4 @@
- #29939: Fix precicion for hyperbolic geodesic
- #29945: Failing doctest in src/sage/coding/linear_code.py (just a wrong doctest, will be fixed here)
- #29954: Unstable plotting
+- #29956: Bug in KlyachkoBundle_class.random_deformation Description changed:
---
+++
@@ -32,4 +32,5 @@
- #29939: Fix precicion for hyperbolic geodesic
- #29945: Failing doctest in src/sage/coding/linear_code.py (just a wrong doctest, will be fixed here)
- #29954: Unstable plotting
-- #29956: Bug in KlyachkoBundle_class.random_deformation
+- #29956: Bug in KlyachkoBundle_class.random_deformation
+- #29957: Bug in ContinuedFraction rounding Description changed:
---
+++
@@ -33,4 +33,5 @@
- #29945: Failing doctest in src/sage/coding/linear_code.py (just a wrong doctest, will be fixed here)
- #29954: Unstable plotting
- #29956: Bug in KlyachkoBundle_class.random_deformation
-- #29957: Bug in ContinuedFraction rounding
+- #29957: Bug in ContinuedFraction rounding
+- #29958: Too many strong articulation points Branch pushed to git repo; I updated commit sha1. New commits:
4c540c6 | make dynamics fuzz ready |
23ed583 | fix doctest in hyperbolic_space/hyperbolic_point |
5283dc4 | use abs tol flag |
7b244c0 | modify doctests to the extend that they hold with fuzz |
84ba6d5 | Merge branch 'public/29936' of git://trac.sagemath.org/sage into public/29935 |
36ea095 | make finance fuzz ready |
bfe9c2a | bug in connectivity |
a170927 | make groups fuzz ready |
It may be too late for this, but I would suggest a staged approach:
- ticket 1: implement optional random seed but don't make it the default
- tickets 2-n: fix doctests when using random seeds in various parts of the Sage library
- ticket n+1: make random seed standard, if that is what people think should be done
Doing everything all at once is likely to lead to a patch bomb, which will lead to merge conflicts and in general be hard to deal with.
There is a reason that I have the fixes grouped by parts of sage. So that isn't spoiled yet.
That completely makes sense. We still need the last ticket to have the bots test everything.
Description changed:
---
+++
@@ -34,4 +34,5 @@
- #29954: Unstable plotting
- #29956: Bug in KlyachkoBundle_class.random_deformation
- #29957: Bug in ContinuedFraction rounding
-- #29958: Too many strong articulation points
+- #29958: Too many strong articulation points
+- #29961: random symbolic expression is completely unstable Branch pushed to git repo; I updated commit sha1. New commits:
38b4557 | another file for combinat |
8ec8298 | make symbolix fuzz ready |
e8d4776 | make schemes fuzz ready |
9b5bef8 | make plot fuzz ready |
98e34d3 | partially make rings fuzz ready |
6f33e4a | partially make matrix fuzz ready |
e6dc532 | partially make modular fuzz ready |
18e980d | partially make modules fuzz ready |
Description changed:
---
+++
@@ -28,6 +28,11 @@
sage -t --warn-long 89.5 --random_seed=273987373473433732996760115183658447263 src/sage/all.py
```
+Schedule:
+
+- Allow fuzzing: #29962
+- Finally make fuzzing default with this ticket.
+
Errors discovered by this ticket:
- #29939: Fix precicion for hyperbolic geodesic
- #29945: Failing doctest in src/sage/coding/linear_code.py (just a wrong doctest, will be fixed here)Description changed:
---
+++
@@ -25,7 +25,7 @@
This can then be reproduced with
```
-sage -t --warn-long 89.5 --random_seed=273987373473433732996760115183658447263 src/sage/all.py
+sage -t --warn-long 89.5 --random-seed=273987373473433732996760115183658447263 src/sage/all.py
```
Schedule:Branch pushed to git repo; I updated commit sha1. New commits:
fc25017 | dash instead of underscore for command line options |
Description changed:
---
+++
@@ -12,7 +12,7 @@
sage -t src/sage/all.py
Running doctests with ID 2020-06-23-23-19-03-8003eea5.
...
-sage -t --warn-long 89.5 --random_seed=273987373473433732996760115183658447263 src/sage/all.py
+sage -t --warn-long 89.5 --random-seed=273987373473433732996760115183658447263 src/sage/all.py
[16 tests, 0.73 s]
----------------------------------------------------------------------
All tests passed!
@@ -31,7 +31,12 @@
Schedule:
- Allow fuzzing: #29962
+- Make all parts of sage ready for default fuzzing:
+ - #29963: geometry
- Finally make fuzzing default with this ticket.
+
+Follow up:
+- Remove `set_random_seed` in doctests where possible.
Errors discovered by this ticket:
- #29939: Fix precicion for hyperbolic geodesicDescription changed:
---
+++
@@ -32,7 +32,30 @@
- Allow fuzzing: #29962
- Make all parts of sage ready for default fuzzing:
- - #29963: geometry
+ - #29945 coding
+ - #29963 geometry
+ - #29964 libs
+ - #29965 graphs
+ - #29967 interfaces
+ - #29968 algebras
+ - #29969 misc
+ - #29970 arith
+ - #29971 categories
+ - #29972 stats
+ - #29973 sets
+ - #29974 combinat
+ - #29975 numerical, probability
+ - #29976 matrix
+ - #29977 modular
+ - #29978 modules
+ - #29979 rings
+ - #29980 crypto
+ - #29981 documentation
+ - #29982 dynamics
+ - #29983 finance
+ - #29984 symbolic
+ - #29985 schemes
+ - #29986 plot
- Finally make fuzzing default with this ticket.
Follow up:We had a little discussion on #29971, how we treat doctests as
sage: C = FiniteEnumeratedSets().example()
sage: C.random_element()
1
If we tag this as # random and maybe test instead containment in C, the doctest we will essentially remove the doctest and add another. Is this what we want?
Matthias Koeppe suggested inventing a new tag maybe # random-seed=0. If we implicitly fuzz all doctests this would only be tested if we run the test block twice?!
I think we should agree on a something here. Depending on the agreement it needs to be taken care of with #29962 already.
IMO tests as C.random_element() should run at least once with fuzzing (with checking that the object is consistent). We can run it twice to obtain our old test, but I would appreciate some help with that.
This will do the job, and doesn't depend on the literal output of the random function:
sage: C = FiniteEnumeratedSets().example()
sage: C.random_element() in C
True
In principal I agree.
I prefer
sage: C = FiniteEnumeratedSets().example()
sage: n = C.random_element(); n # random
1
sage: n in C
True
as this makes it a bit easier for the user to see, what will/might happen. But that is only minor.
However, I just noticed, that there are problems with that approach. The representation of those random things will never be checked. In principal (almost) all random events should be reproducible with some random seed (doesn't work for e.g. hashing). By tagging something as # random, we might document nonsense or outdated information and no one will catch on.
E.g. in #29981 I fixed a documentation mistake along the way: Solve a linear equation system with A\v with A a random matrix. A should be non-singular of course, which was never thought of because the doctest is only tested for one matrix, which happens to be non-singular. In #29981, A is defined as A = random_matrix(QQ, 3, algorithm='unimodular'). However, I forgot to update the representation of A to some unimodular matrix and the doctests never caught on.
Now if you tag something as # random-seed=0 and we find some way to test this (both seed 0 and different), it would mean that the printed representations stay up to date and consistent and it is also tested, if those thing work for "random" input.
I found many examples where functions where tested assuming random seed 0. One could make use of the double testing framework:
def foo(y):
r"""
EXAMPLES::
sage: M = random(); M # random-seed=0
<some random input>
sage: N = foo(M); N # random-seed=0
<some random output>
sage: check_for_consistency(M, N)
"""
return _foo(y)
It is still checked, that foo works exactly for some example that hopefully was verified by someone. But it is also checked, that the method in principal works on "random" input and that the output appears to be correct.
If we want to check the printed/string representation of something, can't we just... check it? In other words, instead of
sage: C = FiniteEnumeratedSets().example()
sage: C.random_element()
1
we could do
sage: C = FiniteEnumeratedSets().example()
sage: C.random_element() in C
True
sage: C[0]
1
This has the (big, in my opinion) added benefit of actually being a runnable example. If a user runs
sage: C = FiniteEnumeratedSets().example()
sage: n = C.random_element(); n # random
then he's not going to see what the documentation says he should see, because # random does nothing on the command-line.
IMO, it is not necessary to test the printing of random elements, as the repr of those elements is already tested elsewhere. We do not make any promises about the precise elements that are returned with a seed of 0, so we should not bake that into the doctests. As we cannot test the randomness, we can only test that random_element returns an element of the parent. The following check could be used for that:
sage: C.random_element().parent() is C
Unlike __contains__, this does not implicitly coerce the element into C.
Replying to @mwageringel:
IMO, it is not necessary to test the printing of random elements, as the repr of those elements is already tested elsewhere. We do not make any promises about the precise elements that are returned with a seed of 0, so we should not bake that into the doctests. As we cannot test the randomness, we can only test that
random_elementreturns an element of the parent. The following check could be used for that:sage: C.random_element().parent() is CUnlike
__contains__, this does not implicitly coerce the element intoC.
Sounds good to me.
Replying to @kliem:
Sounds good to me. As for the documentation of random_element, I don't think a user cares about the precise value of set_random_seed(0); C.random_element().
Two more cases that I would like to discuss.
- Basically from comment:60. How do we deal with something like this:
def foo(y):
r"""
EXAMPLES::
sage: M = random(); M
<some random input>
sage: foo(M)
<some random output>
"""
It appears to me that there are many examples, where someone did not write down a concrete element to doctest. I would propose having two doctests, if possible:
- Explicitly obtain the old test.
- Have a "random" test with a consistency check. (Even without a consistency check, it's probably worth checking that the function terminates without an error usually.)
- The other one is a concrete example from #29981 (linear algrebra tutorial). The original test is
sage: A=random_matrix(QQ,3)
sage: v=vector([2,3,1])
sage: A,v
(
[ 0 -1 1]
[-1 -1 -1]
[ 0 2 2], (2, 3, 1)
)
sage: x=A\v; x
(-7/2, -3/4, 5/4)
sage: A*x
(2, 3, 1)
As mentioned already, this does not work. I see basically two ways of fixing this. One is to explicitly write down A. The other one is similar to what I did. Use a correct random approach (e.g. random unimodular) and check that indeed A*x == v. Optionally one can print x and A and tag this as random. To be honest, I don't see how a user would profit from the printing. What a 3 by 3 matrix looks like should be clear by then.
Replying to @kliem:
- Basically from comment:60. How do we deal with something like this:
def foo(y): r""" EXAMPLES:: sage: M = random(); M <some random input> sage: foo(M) <some random output> """
If the point isn't to demonstrate the random() function, then the "random" element can just be replaced with whatever it winds up with now (i.e. the fixed element you get with a seed of zero).
The TESTS block can contain some truly random test of a property that the output from foo() should satisfy, like
sage: foo(random()) in RR
True
- The other one is a concrete example from #29981 (linear algrebra tutorial). The original test is
sage: A=random_matrix(QQ,3) sage: v=vector([2,3,1]) sage: A,v ( [ 0 -1 1] [-1 -1 -1] [ 0 2 2], (2, 3, 1) ) sage: x=A\v; x (-7/2, -3/4, 5/4) sage: A*x (2, 3, 1)
How about...
sage: A = random_matrix(QQ,3)
sage: while not A.is_invertible():
....: A = random_matrix(QQ,3)
....:
sage: v=vector([2,3,1])
sage: x=A\v
sage: A*x - v == 0
True
Description changed:
---
+++
@@ -68,4 +68,5 @@
- #29956: Bug in KlyachkoBundle_class.random_deformation
- #29957: Bug in ContinuedFraction rounding
- #29958: Too many strong articulation points
-- #29961: random symbolic expression is completely unstable
+- #29961: random symbolic expression is completely unstable
+- #30045: Bug in Reed-Solomon encoder and error-erasure decoder Replying to @orlitzky:
Replying to @kliem:
- Basically from comment:60. How do we deal with something like this:
def foo(y): r""" EXAMPLES:: sage: M = random(); M <some random input> sage: foo(M) <some random output> """If the point isn't to demonstrate the
random()function, then the "random" element can just be replaced with whatever it winds up with now (i.e. the fixed element you get with a seed of zero).
TheTESTSblock can contain some truly random test of a property that the output fromfoo()should satisfy, likesage: foo(random()) in RR True
- The other one is a concrete example from #29981 (linear algrebra tutorial). The original test is
sage: A=random_matrix(QQ,3) sage: v=vector([2,3,1]) sage: A,v ( [ 0 -1 1] [-1 -1 -1] [ 0 2 2], (2, 3, 1) ) sage: x=A\v; x (-7/2, -3/4, 5/4) sage: A*x (2, 3, 1)How about...
sage: A = random_matrix(QQ,3) sage: while not A.is_invertible(): ....: A = random_matrix(QQ,3) ....: sage: v=vector([2,3,1]) sage: x=A\v sage: A*x - v == 0 True
Sounds reasonable to me. I will adhere to those suggestions.
Changed branch from public/29935 to public/29935-reb
New commits:
1d7b00e | dash instead of underscore for command line options |
b31e2d5 | Merge branch 'public/29962' of git://trac.sagemath.org/sage into public/29962-reb |
2f30dd9 | small fixes |
b62f781 | doctests do not start from a random seed by default yet |
263d605 | Revert "doctests do not start from a random seed by default yet" |
3636597 | Revert "default random seed 0 for now" |
Maybe it makes sense to restrict the size of the random seed, so that one can still read the output of doctesting.
Replying to @kliem:
Maybe it makes sense to restrict the size of the random seed, so that one can still read the output of doctesting.
it's not clear to me what you mean here.
sage -t src/sage/all.py
Running doctests with ID 2020-06-23-23-19-03-8003eea5.
...
sage -t --warn-long 89.5 --random-seed=273987373473433732996760115183658447263 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
Currently this output is the plan. This might be very annoying for patchbots and so on. If we instead restrict to fewer digits, we will still test a reasonable amount of different inputs and you can still read this line (even if working on a non-huge screen).
I think it is ok - compare this with very long lists of optional packages which are there sometimes.
Description changed:
---
+++
@@ -62,7 +62,7 @@
- Remove `set_random_seed` in doctests where possible.
Errors discovered by this ticket:
-- #29939: Fix precicion for hyperbolic geodesic
+- #29936 Sign error in hyperbolic geodesic
- #29945: Failing doctest in src/sage/coding/linear_code.py (just a wrong doctest, will be fixed here)
- #29954: Unstable plotting
- #29956: Bug in KlyachkoBundle_class.random_deformationDescription changed:
---
+++
@@ -34,6 +34,7 @@
- Make all parts of sage ready for default fuzzing:
- #29945 coding
- #29963 geometry
+ - #29936 Sign error in hyperbolic geodesic
- #29964 libs
- #29965 graphs
- #29967 interfacesChanged dependencies from #29945, #29963, #29964, #29965, #29967, #29968, #29969, #29970, #29971, #29972, #29973, #29974, #29975, #29976, #29977, #29978, #29979, #29980, #29981, #29982, #29983, #29984, #29985, #29986 to 29936, #29945, #29963, #29964, #29965, #29967, #29968, #29969, #29970, #29971, #29972, #29973, #29974, #29975, #29976, #29977, #29978, #29979, #29980, #29981, #29982, #29983, #29984, #29985, #29986
Description changed:
---
+++
@@ -70,4 +70,5 @@
- #29957: Bug in ContinuedFraction rounding
- #29958: Too many strong articulation points
- #29961: random symbolic expression is completely unstable
-- #30045: Bug in Reed-Solomon encoder and error-erasure decoder
+- #30045: Bug in Reed-Solomon encoder and error-erasure decoder
+- Index error with random derangement (fixed in #29974)Changed dependencies from 29936, #29945, #29963, #29964, #29965, #29967, #29968, #29969, #29970, #29971, #29972, #29973, #29974, #29975, #29976, #29977, #29978, #29979, #29980, #29981, #29982, #29983, #29984, #29985, #29986 to #29936, #29945, #29963, #29964, #29965, #29967, #29968, #29969, #29970, #29971, #29972, #29973, #29974, #29975, #29976, #29977, #29978, #29979, #29980, #29981, #29982, #29983, #29984, #29985, #29986
Work Issues: document range of random-seed
Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.