Make categories doctests ready for random seeds

[kliem]

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

[kliem]

Branch: public/29971

[kliem]

New commits:

da1c6be	start from a "random" random seed for doctesting
b7b836d	make random seed reproducible
eedbe5e	document random_seed
998b1b9	default random seed 0 for now
1d7b00e	dash instead of underscore for command line options
8715b01	make categories fuzz ready

[kliem]

Commit: 8715b01

[mkoeppe]

comment:2

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.

[kliem]

comment:3

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.

[sagetrac-git]

Changed commit from 8715b01 to 382abb9

[sagetrac-git]

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

382abb9

make random tests more meaningful

[kliem]

comment:5

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).

[mkoeppe]

comment:6

Replying to @kliem:

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.

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

[kliem]

comment:7

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.

[mkoeppe]

comment:8

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.

[kliem]

comment:9

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.

[jhpalmieri]

comment:10

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

[kliem]

comment:11

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).

[mkoeppe]

comment:12

I agree, discussing this on sage-devel and #29935 would probably be best.

[kliem]

comment:13

Merge conflict.

I also need to go through it again and check I respected the design decision in #29935.

[kliem]

Changed commit from 382abb9 to d3457de

[kliem]

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

[kliem]

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

[kliem]

comment:15

Note that elements of C = FiniteEnumeratedSets().example() are not elements of C, but integers.

[mwageringel]

Reviewer: Markus Wageringel

[mwageringel]

comment:16

This seems to work.

[kliem]

comment:17

Thank you.

[vbraun]

Changed branch from public/29971-reb to d3457de