Make categories doctests ready for random seeds
Closed this issue · 24 comments
This ticket makes
sage -t --long --random-seed=n src/sage/categories/
pass for different values n than just 0.
Depends on #29962
Component: doctest framework
Author: Jonathan Kliem
Branch/Commit: d3457de
Reviewer: Markus Wageringel
Issue created by migration from https://trac.sagemath.org/ticket/29971
Branch: public/29971
I'm not sure if this is a good way to update the doctests. Essentially you are disabling a test by marking it random; the only way it can fail is if it raises an error.
I think it would be better to invent a new doctest tag such as "# random_seed=0" or whatever is syntactically convenient.
To advertise my point about testing this stuff random #29961. In this instance, the get_random_element thing really failed sometimes. It wasn't discovered before, because only seed 0 is tested.
What's the point about testing this only at seed 0. This doesn't tell us anything about how stable the method is.
But I should add a test that illustrates that this is an element of the correct thing.
Branch pushed to git repo; I updated commit sha1. New commits:
382abb9 | make random tests more meaningful |
Is this better? I would say the test is now more valuable than the old tests. It tells us that random_element will give us an element (not just on random seed 0 and not just something that looks like an element).
Replying to @kliem:
To advertise my point about testing this stuff random #29961. In this instance, the
get_random_elementthing really failed sometimes. It wasn't discovered before, because only seed 0 is tested.What's the point about testing this only at seed 0.
Well, my main point is that test coverage before and after this ticket are really incomparable. The change
- drops the test for seed 0 giving a specific element (or, as you say, what looks like an element);
- adds a test for an arbitrary seed giving no error and passing the
__contains__test
The other thing that one could do, if you really want to keep the old test, is add the old test in the TESTS block (starting with set_random_seed(0)).
This way one could keep the old test.
I'm not sure if I like explicit set_random_seed calls in the doctests because they affect all following doctests in the same block.
I would prefer a solution as proposed in comment 2 above if this can be done easily.
How will the solution as in comment two work out if all testings are done with random seed. That would basically mean, it is never tested as well.
I know that set_random_seed(0) will affect all other tests in the same block. This is why I propsed having it in the tests section (as last thing being run). Than it wouldn't affect anything.
How about a Makefile target that runs tests with random_seed=0, or maybe a Makefile target that runs tests twice, once with seed=0 and once with a random seed? Then make sure that people start thinking of that as the new default.
Or as @kliem suggests, double up on some doctests:
sage: test1
sage: test2
End of test block::
sage: test1 # random-seed=0
sage: test2 # random-seed=0
I feel like the discussion is on the wrong ticket.
This ticket is merely about making tests in categories pass with a different random seed than 0.
In principal a new flag random-seed=0 sounds fine, but shouldn't be introduced here. That should be introduced in #29962 I think, or in a seperate follow up ticket that this here is based on.
The discussion about the big pictures mainly happened on sage-devel and on #29935 so far.
I mean there are 23 tickets like this and we should really come up with a uniform way to do them all. I'm fine with doing those 23 tickets, but if the general set up should be different, I might need some help with #29962 or #29935 (one introduces random seed, one makes them default after 23 tickets made all parts of sage compatible to it).
I agree, discussing this on sage-devel and #29935 would probably be best.
Merge conflict.
I also need to go through it again and check I respected the design decision in #29935.
Changed branch from public/29971 to public/29971-reb
New commits:
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 |
1d99129 | fix merge conflict |
3c38b5d | make categories fuzz ready |
1a6cd3f | make random tests more meaningful |
d3457de | make doctests according to #29935 |
Note that elements of C = FiniteEnumeratedSets().example() are not elements of C, but integers.
Reviewer: Markus Wageringel
This seems to work.
Thank you.
Changed branch from public/29971-reb to d3457de