Opened 2 years ago
Closed 10 months ago
#29935 closed enhancement (fixed)
implicitly fuzz RNGdependent doctests with a random random seed
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  doctest framework  Keywords:  fuzz, random, seed 
Cc:  ghkliem, mjo, ghDaveWitteMorris, slelievre, mantepse  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Michael Orlitzky 
Report Upstream:  N/A  Work issues:  
Branch:  047379c (Commits, GitHub, GitLab)  Commit:  047379c56890113ff59c9eb59276eeca274f4b79 
Dependencies:  #32667  Stopgaps: 
Description (last modified by )
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 nonrandomly!
This is reminiscent of an xkcd comic illustration of random number generators.
Based on these considerations and the related sagedevel 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 randomseed
flag to sage t
, allowing:
sage t long randomseed=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 randomseed=${SEED} ${DIR}" \ && ./sage t long randomseed=${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 202006232319038003eea5. ... sage t warnlong 89.5 randomseed=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 warnlong 89.5 randomseed=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.
Followup:
 #32544: Metaticket: Fix unstable doctests detected after #29935
 Remove
set_random_seed
in 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
ContinuedFraction
rounding  #29958: Too many strong articulation points
 #29961: Random symbolic expression is completely unstable
 #30045: Bug in ReedSolomon encoder and errorerasure decoder
 Index error with random derangement (fixed in #29974)
 #31890: simplify_hypergeometric is unstable
 #31891:
ZeroDivisonError
when creating polynomial system  #31892: Conic parametrization broken
 #32075: Polynomial generic power trunk broken
 #32083: Various errors with polybori including segmentation fault
 #32084
_nth_root_naive
fails 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 2adic 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_field
unstable
Attachments (1)
Change History (185)
comment:1 Changed 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Cc ghkliem mjo added
comment:3 Changed 2 years ago by
 Work issues set to modify coding conventions
comment:4 Changed 2 years ago by
 Dependencies set to #29904
comment:5 followup: ↓ 6 Changed 2 years ago by
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.
comment:6 in reply to: ↑ 5 Changed 2 years ago by
Replying to ghkliem:
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 of0.7
when 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
geometry
module appears to ready.Edit: And there is some place where you need to add
set_random_seed(0)
now or similar.
comment:7 Changed 2 years ago by
 Branch set to public/29935
 Commit set to b2954ceea638b4510e99d90cea3f99fa9a49338a
comment:8 followup: ↓ 11 Changed 2 years ago by
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 1e2 [1 0 0] [0 1 0] [0 0 1]
There is no mentioning there that only one particular matrix is tested.
comment:9 Changed 2 years ago by
 Commit changed from b2954ceea638b4510e99d90cea3f99fa9a49338a to dc49dd0bd06dae1bca0a24b11b94f3f6d0fa2112
Branch pushed to git repo; I updated commit sha1. New commits:
dc49dd0  update developer guide

comment:10 Changed 2 years ago by
 Work issues modify coding conventions deleted
comment:11 in reply to: ↑ 8 Changed 2 years ago by
Replying to ghkliem:
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 1e2 [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
comment:12 Changed 2 years ago by
Nothing to assume there. random_matrix(RR,3,3)
has entries between 1
and 1
. After dividing we are good.
comment:13 Changed 2 years ago by
 Dependencies changed from #29904 to #29904, #29936
comment:14 Changed 2 years ago by
How about making the random seed an option to sageruntests
. Then people can fuzz the doctests if they want.
comment:15 Changed 2 years ago by
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.
comment:16 Changed 2 years ago by
it is not a "real" (timeconsuming) 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.
comment:17 Changed 2 years ago by
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.
comment:18 Changed 2 years ago by
it is a good point  and the nondet. seed needs to be logged  if this is not yet in the branch it should get there.
comment:19 Changed 2 years ago by
So I would suggest to use this ticket to introduce the option, not change the default.
comment:20 Changed 2 years ago by
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.
comment:21 Changed 2 years ago by
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.
comment:22 Changed 2 years ago by
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.
comment:23 Changed 2 years ago by
Feel free to do with the branch whatever you think is reasonable.
comment:24 Changed 2 years ago by
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 20200622185654668a26f3. 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 warnlong 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 warnlong 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.
comment:25 followup: ↓ 32 Changed 2 years ago by
Just to weigh in:
 The default should be to use a randomlychosen seed.
 Failures should be reproducible:
 It should be possible to pass a seed to the test runner with e.g.
seed
.  Any test failures should log/echo the current seed so that the failing test can be reproduced easily using the option in (2.a.).
 It should be possible to pass a seed to the test runner with e.g.
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.
comment:26 followup: ↓ 28 Changed 2 years ago by
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?
comment:27 Changed 2 years ago by
 Commit changed from dc49dd0bd06dae1bca0a24b11b94f3f6d0fa2112 to bac708e0be8cbceb786906d6d1d08a6040775d20
Branch pushed to git repo; I updated commit sha1. New commits:
bac708e  make graphs ready for fuzz doctests

comment:28 in reply to: ↑ 26 Changed 2 years ago by
Replying to ghkliem:
there is plenty of
set_random_seed
for cones doctests? Should I remove those as well or do we do this somewhere else?
Not on the same ticket.
comment:29 Changed 2 years ago by
 Commit changed from bac708e0be8cbceb786906d6d1d08a6040775d20 to b5e9896efbe68fc3a374965ea9be5200e6d99cfb
Branch pushed to git repo; I updated commit sha1. New commits:
b5e9896  make algebras fuzz ready

comment:30 Changed 2 years ago by
 Commit changed from b5e9896efbe68fc3a374965ea9be5200e6d99cfb to 8babc2a801d725b1f1687fac6c2082346778efb7
Branch pushed to git repo; I updated commit sha1. New commits:
8babc2a  make arith fuzz ready

comment:31 Changed 2 years ago by
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.
comment:32 in reply to: ↑ 25 Changed 2 years ago by
Replying to mjo:
 The default should be to use a randomlychosen seed.
 Failures should be reproducible:
 It should be possible to pass a seed to the test runner with e.g.
seed
. 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 Python2 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.
comment:33 Changed 2 years ago by
Apparently LinearCodeSyndromeDecoder.decode_to_code
is broken.
See #29945.
comment:34 Changed 2 years ago by
 Commit changed from 8babc2a801d725b1f1687fac6c2082346778efb7 to c7d16e9e58748d7627eb891df152ed9f2beafb76
comment:35 Changed 2 years ago by
 Commit changed from c7d16e9e58748d7627eb891df152ed9f2beafb76 to e5c54af8747ceb223147c4b84975e8773d3c0c04
comment:36 Changed 2 years ago by
 Description modified (diff)
comment:37 Changed 2 years ago by
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.
comment:38 Changed 2 years ago by
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/sitepackages/sage/misc/sage_unittest.py", line 296, in run test_method(tester=tester) File "/home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/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/sitepackages/sage/misc/sage_unittest.py", line 296, in run test_method(tester=tester) File "/home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/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/sitepackages/sage/misc/sage_unittest.py", line 296, in run test_method(tester=tester) File "/home/jonathan/Applications/sage/local/lib/python3.7/sitepackages/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
comment:39 Changed 2 years ago by
 Commit changed from e5c54af8747ceb223147c4b84975e8773d3c0c04 to 93415f39a9691a3815e46ab5a4b51835b78b34fd
Branch pushed to git repo; I updated commit sha1. New commits:
93415f3  make combinat fuzz ready

comment:40 Changed 2 years ago by
 Commit changed from 93415f39a9691a3815e46ab5a4b51835b78b34fd to dbd7f6ce5d5e631445443d24be2b070a4ea8a7c1
Branch pushed to git repo; I updated commit sha1. New commits:
dbd7f6c  fix doctest in sage/coding/linear_code

comment:41 Changed 2 years ago by
 Description modified (diff)
comment:42 Changed 2 years ago by
 Commit changed from dbd7f6ce5d5e631445443d24be2b070a4ea8a7c1 to 00dc135d18892c4310013a630757423d6f31605f
comment:43 Changed 2 years ago by
In 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.0e13 1298 (0.023437500000000003, 1299 0.03415488992713045, 1300 0.4627803348994754, 1301 0.9345951100728696) 1302 sage: G.position(1) # tol 1.0e13 1303 (0.5136230468749999, 1304 0.19293222169724827, 1305 0.46278033489947534, 1306 0.617040446532634)
when running on set_random_seed(151058820726654196682836430928254760259)
. The precision is somewhere around 1e3
then. Is this a bug?
comment:44 Changed 2 years ago by
 Description modified (diff)
comment:45 Changed 2 years ago by
 Description modified (diff)
comment:46 Changed 2 years ago by
 Description modified (diff)
comment:47 Changed 2 years ago by
 Commit changed from 00dc135d18892c4310013a630757423d6f31605f to a17092718667b3c9598939391cbcf36e1169c996
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

comment:48 Changed 2 years ago by
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 2n: 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.
comment:49 Changed 2 years ago by
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.
comment:50 Changed 2 years ago by
 Description modified (diff)
comment:51 Changed 2 years ago by
 Commit changed from a17092718667b3c9598939391cbcf36e1169c996 to 18e980dc63337149b1e708a2078ec2978f0ead7d
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

comment:52 Changed 2 years ago by
 Dependencies changed from #29904, #29936 to #29904, #29936, #29962
 Description modified (diff)
comment:53 Changed 2 years ago by
 Commit changed from 18e980dc63337149b1e708a2078ec2978f0ead7d to 0a4f42f4dcf7531c35bd32dd4ab2dcc724f26712
comment:54 Changed 2 years ago by
 Description modified (diff)
comment:55 Changed 2 years ago by
 Commit changed from 0a4f42f4dcf7531c35bd32dd4ab2dcc724f26712 to fc25017e8ea86bb4bfac215ce1471952087d6235
Branch pushed to git repo; I updated commit sha1. New commits:
fc25017  dash instead of underscore for command line options

comment:56 Changed 2 years ago by
 Description modified (diff)
New commits:
fc25017  dash instead of underscore for command line options

comment:57 Changed 2 years ago by
 Dependencies changed from #29904, #29936, #29962 to #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 modified (diff)
comment:58 Changed 2 years ago by
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 # randomseed=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.
comment:59 Changed 2 years ago by
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
comment:60 followups: ↓ 62 ↓ 64 Changed 2 years ago by
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 nonsingular of course, which was never thought of because the doctest is only tested for one matrix, which happens to be nonsingular. 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 # randomseed=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 # randomseed=0 <some random input> sage: N = foo(M); N # randomseed=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.
comment:61 Changed 2 years ago by
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 commandline.
comment:62 in reply to: ↑ 60 ; followup: ↓ 63 Changed 2 years ago by
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
.
comment:63 in reply to: ↑ 62 Changed 2 years ago by
Replying to ghmwageringel:
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 CUnlike
__contains__
, this does not implicitly coerce the element intoC
.
Sounds good to me.
comment:64 in reply to: ↑ 60 ; followup: ↓ 65 Changed 2 years ago by
Replying to ghkliem:
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 downA
. The other one is similar to what I did. Use a correct random approach (e.g. random unimodular) and check that indeedA*x == v
. Optionally one can printx
andA
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.
comment:65 in reply to: ↑ 64 ; followup: ↓ 67 Changed 2 years ago by
Replying to ghkliem:
 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
comment:66 Changed 2 years ago by
 Description modified (diff)
comment:67 in reply to: ↑ 65 Changed 2 years ago by
Replying to mjo:
Replying to ghkliem:
 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). TheTESTS
block 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.
comment:68 Changed 2 years ago by
 Branch changed from public/29935 to public/29935reb
 Commit changed from fc25017e8ea86bb4bfac215ce1471952087d6235 to 36365975b2d783a16acc15f035d3c893a04d3189
New commits:
1d7b00e  dash instead of underscore for command line options

b31e2d5  Merge branch 'public/29962' of git://trac.sagemath.org/sage into public/29962reb

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"

comment:69 followup: ↓ 70 Changed 2 years ago by
Maybe it makes sense to restrict the size of the random seed, so that one can still read the output of doctesting.
comment:70 in reply to: ↑ 69 Changed 2 years ago by
Replying to ghkliem:
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.
comment:71 Changed 2 years ago by
sage t src/sage/all.py Running doctests with ID 202006232319038003eea5. ... sage t warnlong 89.5 randomseed=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 nonhuge screen).
comment:72 Changed 2 years ago by
I think it is ok  compare this with very long lists of optional packages which are there sometimes.
comment:73 Changed 2 years ago by
 Description modified (diff)
comment:74 Changed 2 years ago by
 Dependencies changed 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 modified (diff)
comment:75 Changed 2 years ago by
 Description modified (diff)
comment:76 Changed 2 years ago by
 Dependencies changed 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
comment:77 Changed 2 years ago by
 Work issues set to document range of randomseed
comment:78 Changed 22 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:79 Changed 19 months ago by
 Cc ghDaveWitteMorris added
comment:80 Changed 17 months ago by
 Milestone changed from sage9.3 to sage9.4
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.
comment:81 Changed 16 months ago by
 Cc slelievre added
 Description modified (diff)
 Keywords fuzz random seed added
 Summary changed from implicitly fuzz RNGdependent doctests to Metaticket: implicitly fuzz RNGdependent doctests with a random random seed
Marking this as a metaticket.
comment:82 Changed 15 months ago by
 Description modified (diff)
comment:83 Changed 15 months ago by
 Description modified (diff)
comment:84 Changed 15 months ago by
 Description modified (diff)
comment:85 Changed 14 months ago by
 Description modified (diff)
Expanding the ticket description, adding a code snippet giving a way to set a random randomness seed, which I find useful for working on the (few remaining) dependencies of this ticket.
comment:86 Changed 14 months ago by
 Description modified (diff)
comment:87 Changed 14 months ago by
 Description modified (diff)
comment:88 Changed 14 months ago by
 Description modified (diff)
comment:89 Changed 14 months ago by
 Description modified (diff)
comment:90 Changed 14 months ago by
 Description modified (diff)
comment:91 Changed 14 months ago by
 Dependencies changed 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 #29979, #29978, #29977, #29980
comment:92 Changed 14 months ago by
 Status changed from new to needs_review
 Summary changed from Metaticket: implicitly fuzz RNGdependent doctests with a random random seed to implicitly fuzz RNGdependent doctests with a random random seed
comment:93 Changed 14 months ago by
 Status changed from needs_review to needs_work
A couple of new tests fail yet.
comment:94 Changed 14 months ago by
 Description modified (diff)
comment:95 Changed 14 months ago by
 Dependencies changed from #29979, #29978, #29977, #29980 to #29979, #29978, #29977, #29980, #32084
comment:96 Changed 14 months ago by
 Dependencies changed from #29979, #29978, #29977, #29980, #32084 to #29979, #29978, #29977, #29980, #32084, 32107
 Description modified (diff)
comment:97 Changed 14 months ago by
 Dependencies changed from #29979, #29978, #29977, #29980, #32084, 32107 to #29979, #29978, #29977, #29980, #32084, #32107
comment:98 Changed 14 months ago by
 Description modified (diff)
comment:99 Changed 14 months ago by
 Dependencies changed from #29979, #29978, #29977, #29980, #32084, #32107 to #29979, #29978, #29977, #29980, #32084, #32107, 980, #32084, #32108
comment:100 Changed 14 months ago by
 Dependencies changed from #29979, #29978, #29977, #29980, #32084, #32107, 980, #32084, #32108 to #29979, #29978, #29977, #29980, #32084, #32107, 980, #32084, #32108, #32109
 Description modified (diff)
comment:101 Changed 14 months ago by
 Dependencies changed from #29979, #29978, #29977, #29980, #32084, #32107, 980, #32084, #32108, #32109 to #29979, #29978, #29977, #29980, #32084, #32107, #32084, #32108, #32109
comment:102 Changed 14 months ago by
 Dependencies changed from #29979, #29978, #29977, #29980, #32084, #32107, #32084, #32108, #32109 to #29979, #29978, #29977, #29980, #32084, #32107, #32108, #32109
comment:103 Changed 14 months ago by
 Branch changed from public/29935reb to public/29935
 Commit changed from 36365975b2d783a16acc15f035d3c893a04d3189 to 453ffc80512f229d1c7bae2581cc7d0500640b22
 Dependencies changed from #29979, #29978, #29977, #29980, #32084, #32107, #32108, #32109 to #29979, #29977, #29980, #32084, #32107, #32108, #32109
 Work issues document range of randomseed deleted
Last 10 new commits:
0994f0f  trac 32084 fix nth root

226fe84  trac 32084 fix nth root

71ab842  Merge branch 'public/32084r2' of git://trac.sagemath.org/sage into public/29935

960ce8b  make groups ready for random seeds

5592c58  make some_elements of affine group ready for random seeds

dc4ed63  Merge branch 'public/32107' of git://trac.sagemath.org/sage into public/29935

a732a3f  fix random tree on less than two vertices

5b19d31  Merge branch 'public/32108' of git://trac.sagemath.org/sage into public/29935

9e53965  fix 0/0 in ore function field

453ffc8  Merge branch 'public/32109' of git://trac.sagemath.org/sage into public/29935

comment:104 Changed 14 months ago by
 Commit changed from 453ffc80512f229d1c7bae2581cc7d0500640b22 to f16765a4ac7806da4401b058f80e9737cca24c3d
comment:105 Changed 14 months ago by
 Dependencies changed from #29979, #29977, #29980, #32084, #32107, #32108, #32109 to #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095
comment:106 Changed 14 months ago by
 Description modified (diff)
comment:107 Changed 14 months ago by
 Description modified (diff)
comment:108 Changed 14 months ago by
 Description modified (diff)
comment:109 Changed 14 months ago by
 Commit changed from f16765a4ac7806da4401b058f80e9737cca24c3d to 97e015ccd9b2160a84db686658e9ec7876893f17
Branch pushed to git repo; I updated commit sha1. New commits:
75fad77  Merge branch 'public/29979' of git://trac.sagemath.org/sage into public/29979reb

e280083  Merge branch 'public/32084r2' of git://trac.sagemath.org/sage into public/29979reb

192b564  add ticked number to test

b20059c  Merge branch 'public/32109' of git://trac.sagemath.org/sage into public/29979reb

ce98d48  two more doctests

34a233d  pull in 32117

cfe869b  modulo n better be for n > 0

3752b9c  some more remaining tests

97e015c  Merge branch 'public/29979reb' of git://trac.sagemath.org/sage into public/29935

comment:110 Changed 14 months ago by
 Description modified (diff)
comment:111 Changed 14 months ago by
 Description modified (diff)
comment:112 Changed 14 months ago by
 Description modified (diff)
comment:113 Changed 14 months ago by
 Description modified (diff)
comment:114 Changed 14 months ago by
 Description modified (diff)
comment:115 Changed 14 months ago by
 Description modified (diff)
comment:116 Changed 14 months ago by
 Description modified (diff)
comment:117 Changed 14 months ago by
 Commit changed from 97e015ccd9b2160a84db686658e9ec7876893f17 to 7f39bf05c30856655554d656b7079153420b9edc
comment:118 Changed 14 months ago by
 Commit changed from 7f39bf05c30856655554d656b7079153420b9edc to cb037caa90caffd1eac962f5ca05f5acd37c2a35
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cb037ca  fix orbit lenght of gyration

comment:119 Changed 14 months ago by
 Commit changed from cb037caa90caffd1eac962f5ca05f5acd37c2a35 to 01f98ac411218ecd1ff02e6ce79f084f6f153c1a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
01f98ac  fix orbit lenght of gyration

comment:120 Changed 14 months ago by
 Commit changed from 01f98ac411218ecd1ff02e6ce79f084f6f153c1a to b8e9e8eceabcd942fc1fe4b42d9fa3df9ba1cd15
comment:121 Changed 14 months ago by
 Commit changed from b8e9e8eceabcd942fc1fe4b42d9fa3df9ba1cd15 to 49ddba993d062a9036f3130f8c68c0e0f93d29dd
comment:122 Changed 14 months ago by
 Commit changed from 49ddba993d062a9036f3130f8c68c0e0f93d29dd to 431397b861d0ff8324d49949b908fa8200389a75
Branch pushed to git repo; I updated commit sha1. New commits:
aa5276a  trac #32131: fix cutwidth for single edge graph

8488bf4  trac #32131: fix method for non connected graphs

8d9abe0  Merge branch 'public/graphs/32131_fix_cutwidth' of git://trac.sagemath.org/sage into public/29935

431397b  a few more tests

comment:123 Changed 14 months ago by
 Dependencies changed from #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095 to #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131
comment:124 Changed 14 months ago by
 Description modified (diff)
comment:125 Changed 14 months ago by
 Description modified (diff)
comment:126 Changed 14 months ago by
 Commit changed from 431397b861d0ff8324d49949b908fa8200389a75 to 692aba8371d7808ba571504210a6b3607e033a59
Branch pushed to git repo; I updated commit sha1. New commits:
ac0dca7  one more missing test from groups

fcdc025  one more test from modular

008c6e1  one more failing test in coding

5f0f5dc  29979: mark failing tests as not tested

bc1e14a  29979: minor tweaks of doctests

692aba8  Merge branch 'public/29979reb' of git://trac.sagemath.org/sage into public/29935

comment:127 Changed 14 months ago by
 Commit changed from 692aba8371d7808ba571504210a6b3607e033a59 to 2ca52c60cc3dbc5356e9b6f8f4599f64ebae3d7b
Branch pushed to git repo; I updated commit sha1. New commits:
2ca52c6  two more doctests

comment:128 Changed 14 months ago by
 Status changed from needs_work to needs_review
This is getting more and more stable.
I can now run it on 8 random seeds without failure (or without failure that doesn't appear on random seed 0). There are probably still failing examples, but its getting to a point, where it won't prevent patchbots from working and it's not too much of a burden to report failures because of this.
comment:129 Changed 14 months ago by
We should also remember to try the optional tests at some point, but I assume the majority of them does not use randomness.
comment:130 Changed 14 months ago by
Right, I forgot about the optional packages.
I hope this helps a bit:
comment:131 Changed 13 months ago by
 Description modified (diff)
comment:132 Changed 13 months ago by
 Commit changed from 2ca52c60cc3dbc5356e9b6f8f4599f64ebae3d7b to 20336ccf999c8aa7961312b25b7ac78076a03d95
Branch pushed to git repo; I updated commit sha1. New commits:
20336cc  various small fixes

comment:133 Changed 13 months ago by
 Commit changed from 20336ccf999c8aa7961312b25b7ac78076a03d95 to 5f4cabad17709abd0925203e1013bce7bfbf5ecc
Branch pushed to git repo; I updated commit sha1. New commits:
836be04  remove bug tag as 32169 has been resolved

7f082ee  a few more fixes for various doctests

76ce723  trac #32169: new ILP formulation

047afda  trac #32169: enforce equality in the number of incoming edges

5f4caba  Merge branch 'public/graphs/32169_new_ILP' of git://trac.sagemath.org/sage into public/29935

comment:134 Changed 13 months ago by
 Dependencies changed from #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131 to #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169
comment:135 Changed 13 months ago by
 Commit changed from 5f4cabad17709abd0925203e1013bce7bfbf5ecc to 474490ef8a950f5867d581e6dff11afc5613b005
Branch pushed to git repo; I updated commit sha1. New commits:
474490e  fix ValueError when obtaining random bounded tolerance graph

comment:136 Changed 13 months ago by
 Commit changed from 474490ef8a950f5867d581e6dff11afc5613b005 to f0f9cd58cbb130a530366c70b666c12e02576241
Branch pushed to git repo; I updated commit sha1. New commits:
f0f9cd5  two more doctests

comment:137 Changed 13 months ago by
 Dependencies changed from #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169 to #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169, #32186
 Description modified (diff)
comment:138 Changed 13 months ago by
 Dependencies changed from #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169, #32186 to #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169, #32186, #32188
 Description modified (diff)
I outsourced many of the small fixes to #32188, to make review here easier.
comment:139 followup: ↓ 141 Changed 13 months ago by
Note that doctesting with a fixed seed is publicly documented behaviour in sage. Quoting from Developer guide:
However, most functions generating pseudorandom output do not need this tag since the doctesting framework guarantees the state of the pseudorandom number generators (PRNGs) used in Sage for a given doctest.
I would think that tests explicitly relying on this behaviour are rather weak and I wouldn't mind changing this "policy", but we shouldn't forget to update the developer guide as well.
comment:140 Changed 13 months ago by
 Dependencies changed from #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169, #32186, #32188 to #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169, #32186, #32188, #32216
 Description modified (diff)
comment:141 in reply to: ↑ 139 Changed 13 months ago by
Replying to nbruin:
Note that doctesting with a fixed seed is publicly documented behaviour in sage. Quoting from Developer guide:
However, most functions generating pseudorandom output do not need this tag since the doctesting framework guarantees the state of the pseudorandom number generators (PRNGs) used in Sage for a given doctest.
I would think that tests explicitly relying on this behaviour are rather weak and I wouldn't mind changing this "policy", but we shouldn't forget to update the developer guide as well.
Thanks for the pointer. This is now #32216.
comment:142 Changed 12 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:143 Changed 11 months ago by
 Branch changed from public/29935 to public/29935reb
 Commit changed from f0f9cd58cbb130a530366c70b666c12e02576241 to 84ca9564b4dd0466e0c2078f651970ece6fc4fad
 Dependencies #29979, #29977, #29980, #32084, #32107, #32108, #32109, #32095, #32131, #32169, #32186, #32188, #32216 deleted
comment:144 Changed 11 months ago by
I think this,
....: n = ZZ.random_element().abs() + 5
+ ....: if n > 5000: # avoid timeouts
+ ....: continue
can be simplified by passing (5,5001)
as the bound to ZZ.random_element()
, without abs.
And the fix for this has already been merged:
+ sage: test_symbolic_expression_order(10000) # long time, # not tested, known bug (see :trac:`32185`)
comment:145 Changed 11 months ago by
 Commit changed from 84ca9564b4dd0466e0c2078f651970ece6fc4fad to 5405f18a8cb56b0e4654ebc16a5e647dbdbe8a1a
comment:146 Changed 11 months ago by
 Commit changed from 5405f18a8cb56b0e4654ebc16a5e647dbdbe8a1a to 950eaeb6b02cc9da0ce9160e9921112a07720a06
Branch pushed to git repo; I updated commit sha1. New commits:
950eaeb  fix unstable doctests

comment:147 Changed 11 months ago by
 Commit changed from 950eaeb6b02cc9da0ce9160e9921112a07720a06 to 512a10d844686464b8c4686c13e8003d8fd41f67
Branch pushed to git repo; I updated commit sha1. New commits:
512a10d  fixed some doctests for disjoint spanning trees

comment:148 followup: ↓ 149 Changed 11 months ago by
 Reviewers set to https://github.com/orlitzky/sage/actions/runs/1250536890, https://github.com/orlitzky/sage/actions/runs/1250536887
Looks OK now, but there's really no way to be 100% certain. We will probably discover a few instances where e.g. a ptestlong happens to generate four relatively hard problems at once, leading to intermittent timeouts. But regardless, I think the sooner we can get this in, the better. I started some GH actions for added test cycles.
comment:149 in reply to: ↑ 148 Changed 11 months ago by
comment:150 Changed 11 months ago by
 Commit changed from 512a10d844686464b8c4686c13e8003d8fd41f67 to 1d255256033dbdc7dd7480edd162dd86f23a7629
Branch pushed to git repo; I updated commit sha1. New commits:
1d25525  fix unstable doctest in book_stein_ent

comment:151 Changed 11 months ago by
I also think this is ready. It is rather stable now.
We should open a new (meta?) ticket, to collect all those failures, I think.
comment:152 Changed 11 months ago by
 Reviewers changed from https://github.com/orlitzky/sage/actions/runs/1250536890, https://github.com/orlitzky/sage/actions/runs/1250536887 to Michael Orlitzky
 Status changed from needs_review to positive_review
Let's just do it before beta2 happens.
Thanks for all your work on this, I never had the courage to try it myself.
comment:153 Changed 11 months ago by
Thank you.
comment:154 Changed 11 months ago by
 Description modified (diff)
comment:155 Changed 11 months ago by
 Status changed from positive_review to needs_work
sage t long warnlong 45.3 randomseed=58740356065403116296055203326248101103 src/sage/graphs/generic_graph.py ********************************************************************** File "src/sage/graphs/generic_graph.py", line 23499, in sage.graphs.generic_graph.GenericGraph.edge_polytope Failed example: P.is_combinatorially_isomorphic(product(components)) Expected: True Got: False ********************************************************************** 1 item had failures: 1 of 24 in sage.graphs.generic_graph.GenericGraph.edge_polytope [3598 tests, 1 failure, 22.29 s]  sage t long warnlong 45.3 randomseed=58740356065403116296055203326248101103 src/sage/graphs/generic_graph.py # 1 doctest failed 
comment:156 Changed 11 months ago by
 Cc mantepse added
So this ticket got rejected because it detected my own mistake in #32498. That's fair I guess.
What I claimed is complete nonsense, but it did work for seed 0 I guess. The (symmetric) edge polytope of a graph is of course the join of the (symmetric) edge polytopes of the connecting compoments.
comment:157 Changed 11 months ago by
No, that is not the join, it is the subdirect sum. It is really worth reading the definitions once in a while...
comment:158 Changed 11 months ago by
 Commit changed from 1d255256033dbdc7dd7480edd162dd86f23a7629 to 7e487e5a7b5ae14ba862763217f4a2fe27485e79
comment:159 Changed 11 months ago by
 Status changed from needs_work to needs_review
@Martin Rubey: Can you please check the fix of #32498, everything else should still be fine.
comment:160 Changed 11 months ago by
I'm afraid there is a MemoryError
reported by the bot. I'll check whether it is reproducible.
comment:161 Changed 11 months ago by
sage t randomseed=306431873350217748498615142104107167501 src/sage/graphs/generic_graph.py ********************************************************************** File "src/sage/graphs/generic_graph.py", line 23601, in sage.graphs.generic_graph.GenericGraph.symmetric_edge_polytope Failed example: P.is_combinatorially_isomorphic(P1.subdirect_sum(P2)) Exception raised: Traceback (most recent call last): File "/home/martin/sagedevelop/local/lib/python3.8/sitepackages/sage/doctest/forker.py", line 694, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/martin/sagedevelop/local/lib/python3.8/sitepackages/sage/doctest/forker.py", line 1088, in compile_and_execute exec(compiled, globs) File "<doctest sage.graphs.generic_graph.GenericGraph.symmetric_edge_polytope[20]>", line 1, in <module> P.is_combinatorially_isomorphic(P1.subdirect_sum(P2)) File "/home/martin/sagedevelop/local/lib/python3.8/sitepackages/sage/geometry/polyhedron/base.py", line 10715, in is_combinatorially_isomorphic return G_self.is_isomorphic(G_other) File "/home/martin/sagedevelop/local/lib/python3.8/sitepackages/sage/graphs/generic_graph.py", line 22672, in is_isomorphic isom = isomorphic(GC, GC2, partition, partition2, (self._directed or self.has_loops()), 1) File "sage/groups/perm_gps/partn_ref/refinement_graphs.pyx", line 163, in sage.groups.perm_gps.partn_ref.refinement_graphs.isomorphic (build/cythonized/sage/groups/perm_gps/partn_ref/refinement_graphs.c:6775) cdef bint isomorphic = double_coset(<void *>GS1, <void *>GS2, part, ordering, n, &all_children_are_equivalent, &refine_by_degree, &compare_graphs, NULL, NULL, output) File "sage/groups/perm_gps/partn_ref/double_coset.pyx", line 364, in sage.groups.perm_gps.partn_ref.double_coset.double_coset (build/cythonized/sage/groups/perm_gps/partn_ref/double_coset.c:5618) raise MemoryError MemoryError ********************************************************************** 1 item had failures: 1 of 53 in sage.graphs.generic_graph.GenericGraph.symmetric_edge_polytope [3558 tests, 1 failure, 42.09 s]  sage t randomseed=306431873350217748498615142104107167501 src/sage/graphs/generic_graph.py # 1 doctest failed  Total time for all tests: 43.1 seconds cpu time: 40.1 seconds cumulative wall time: 42.1 seconds Pytest is not installed, skip checking tests that rely on it.
comment:162 Changed 11 months ago by
Looks like I need to reduce the number of vertices.
comment:163 Changed 11 months ago by
 Commit changed from 7e487e5a7b5ae14ba862763217f4a2fe27485e79 to 150225d2a183b76540bf06da75084b04959e65f2
Branch pushed to git repo; I updated commit sha1. New commits:
150225d  reasonable graph size for doctets

comment:164 Changed 11 months ago by
Previously I decomposed random graphs of up to 12 vertices. Now I take the disjoint union of random graphs and need to go down with the number of vertices.
comment:165 Changed 11 months ago by
It seems that there is still a problem. With some seeds, testing generic_graph.py
takes about 16 seconds on my computer. However,
sage t randomseed=236024484355828254371843550334759500607 src/sage/graphs/generic_graph.py
times out. I attached the log. The last line executed is
sage: trees = g.edge_disjoint_spanning_trees(k) ## line 6282 ##
Of course, this should not have anything to do with the SEP.
comment:166 Changed 11 months ago by
Yes, that is #32169. I was wondering why it did not happen in this case.
comment:167 Changed 11 months ago by
 Commit changed from 150225d2a183b76540bf06da75084b04959e65f2 to 2609cb529eeb3ce1d3c5c4be41c54dff0f2f03dc
Branch pushed to git repo; I updated commit sha1. New commits:
2609cb5  edge disjoint spanning tree not as fast as claimed, see #32169

comment:168 Changed 10 months ago by
 Commit changed from 2609cb529eeb3ce1d3c5c4be41c54dff0f2f03dc to 138f038113e191703a14b153d31e627ba388c7b5
Branch pushed to git repo; I updated commit sha1. New commits:
138f038  fix doctest failure for random matrix

comment:169 Changed 10 months ago by
 Commit changed from 138f038113e191703a14b153d31e627ba388c7b5 to 8117091a8217210dd1221a212cfa3cde9667f0c1
Branch pushed to git repo; I updated commit sha1. New commits:
8117091  one more unstable doctest

comment:170 Changed 10 months ago by
 Description modified (diff)
New commits:
8117091  one more unstable doctest

comment:171 Changed 10 months ago by
 Commit changed from 8117091a8217210dd1221a212cfa3cde9667f0c1 to 5739ef7ff468e4186ac352dbafaf7a7593b4b97f
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
db8daf9  implicitly fuzz RNGdependent doctests

e1a487f  reduce vertices for edge disjoint spanning trees

d311adf  do not remove fixed doctest

accdc2b  simplify doctest

4ad6665  fix unstable doctests

c2326fc  fixed some doctests for disjoint spanning trees

361b203  fix unstable doctest in book_stein_ent

e45603f  edge disjoint spanning tree not as fast as claimed, see #32169

eba3bc7  fix doctest failure for random matrix

5739ef7  one more unstable doctest

comment:172 Changed 10 months ago by
 Dependencies set to #32498
Rebased on #32498, which fixes the error with (symmetric) edge polytopes.
comment:174 Changed 10 months ago by
 Status changed from needs_review to positive_review
comment:175 Changed 10 months ago by
Thank you.
comment:176 followup: ↓ 179 Changed 10 months ago by
No problem, you'll have to rebase this onto the typofix too I think.
comment:177 Changed 10 months ago by
 Commit changed from 5739ef7ff468e4186ac352dbafaf7a7593b4b97f to 2c478d1c375f10f4e6f7e95fcb71feff74d13653
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. Last 10 new commits:
3cfe235  implicitly fuzz RNGdependent doctests

1980561  reduce vertices for edge disjoint spanning trees

e6dd027  do not remove fixed doctest

22c2b66  simplify doctest

e32986c  fix unstable doctests

7b1bd36  fixed some doctests for disjoint spanning trees

812b555  fix unstable doctest in book_stein_ent

74e505b  edge disjoint spanning tree not as fast as claimed, see #32169

44cd7ae  fix doctest failure for random matrix

2c478d1  one more unstable doctest

comment:178 Changed 10 months ago by
 Status changed from needs_review to positive_review
comment:179 in reply to: ↑ 176 Changed 10 months ago by
Replying to mjo:
No problem, you'll have to rebase this onto the typofix too I think.
I don't know, if this is necessary, but it sure doesn't hurt.
comment:180 Changed 10 months ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:181 Changed 10 months ago by
This is the merge conflict, which permits a trivial solution (at least trivial for a human being):
diff cc src/bin/sageruntests index d1fe6567e7,4fc2062b15..0000000000  a/src/bin/sageruntests +++ b/src/bin/sageruntests @@@ 64,59 55,60 +55,97 @@@ if __name__ == "__main__" 'if "external" is listed, will also run tests for available external software; ' 'if "build" is listed, will also run tests specific to Sage\'s build/packaging system; ' 'if set to "all", then all tests will be run') ++<<<<<<< HEAD + parser.add_option("randorder", type=int, metavar="SEED", help="randomize order of tests") + parser.add_option("randomseed", dest="random_seed", type=int, metavar="SEED", help="random seed (integer) for fuzzing doctests") + parser.add_option("globaliterations", "global_iterations", type=int, default=0, help="repeat the whole testing process this many times") + parser.add_option("fileiterations", "file_iterations", type=int, default=0, help="repeat each file this many times, stopping on the first failure") + parser.add_option("environment", type=str, default="sage.repl.ipython_kernel.all_jupyter", help="name of a module that provides the global environment for tests") + + parser.add_option("i", "initial", action="store_true", default=False, help="only show the first failure in each file") + parser.add_option("exitfirst", action="store_true", default=False, help="end the test run immediately after the first failure or unexpected exception") + parser.add_option("force_lib", "forcelib", action="store_true", default=False, help="do not import anything from the tested file(s)") + parser.add_option("abspath", action="store_true", default=False, help="print absolute paths rather than relative paths") + parser.add_option("verbose", action="store_true", default=False, help="print debugging output during the test") + parser.add_option("d", "debug", action="store_true", default=False, help="drop into a python debugger when an unexpected error is raised") + parser.add_option("onlyerrors", action="store_true", default=False, help="only output failures, not test successes") + + parser.add_option("gdb", action="store_true", default=False, help="run doctests under the control of gdb") + parser.add_option("valgrind", "memcheck", action="store_true", default=False, + help="run doctests using Valgrind's memcheck tool. The log " + "files are named sagememcheck.PID and can be found in " + + os.path.join(DOT_SAGE, "valgrind")) + parser.add_option("massif", action="store_true", default=False, + help="run doctests using Valgrind's massif tool. The log " + "files are named sagemassif.PID and can be found in " + + os.path.join(DOT_SAGE, "valgrind")) + parser.add_option("cachegrind", action="store_true", default=False, + help="run doctests using Valgrind's cachegrind tool. The log " + "files are named sagecachegrind.PID and can be found in " + + os.path.join(DOT_SAGE, "valgrind")) + parser.add_option("omega", action="store_true", default=False, + help="run doctests using Valgrind's omega tool. The log " + "files are named sageomega.PID and can be found in " + + os.path.join(DOT_SAGE, "valgrind")) + + parser.add_option("f", "failed", action="store_true", default=False, ++======= + parser.add_argument("randorder", type=int, metavar="SEED", help="randomize order of tests") + parser.add_argument("randomseed", dest="random_seed", type=int, metavar="SEED", help="random seed for fuzzing doctests") + parser.add_argument("globaliterations", "global_iterations", type=int, default=0, help="repeat the whole testing process this many times") + parser.add_argument("fileiterations", "file_iterations", type=int, default=0, help="repeat each file this many times, stopping on the first failure") + parser.add_argument("environment", type=str, default="sage.repl.ipython_kernel.all_jupyter", help="name of a module that provides the global environment for tests") + + parser.add_argument("i", "initial", action="store_true", default=False, help="only show the first failure in each file") + parser.add_argument("exitfirst", action="store_true", default=False, help="end the test run immediately after the first failure or unexpected exception") + parser.add_argument("force_lib", "forcelib", action="store_true", default=False, help="do not import anything from the test ...
comment:182 Changed 10 months ago by
 Commit changed from 2c478d1c375f10f4e6f7e95fcb71feff74d13653 to 047379c56890113ff59c9eb59276eeca274f4b79
Branch pushed to git repo; I updated commit sha1. New commits:
047379c  fix merge conflict

comment:183 Changed 10 months ago by
 Status changed from needs_work to positive_review
comment:184 Changed 10 months ago by
 Branch changed from public/29935reb to 047379c56890113ff59c9eb59276eeca274f4b79
 Resolution set to fixed
 Status changed from positive_review to closed
Just so that we don't forget that we will have to update
https://doc.sagemath.org/html/en/developer/coding_basics.html