Deal with Random changing attributes of object

[ChrisJefferson]

This changes the tester to add one initial call to Random when testing random generation. This deals with the problem where the first call to Random adds attributes, which changes which Random method gets called. Therefore calling Random twice, even if we reset GlobalMersenneTwister, produces different values.

The other alternative (why I marked this discussion) is that we could say this is a bug, and different overloads must provide the same sequence of random values.

[fingolfin]

Could you give a concrete example where this takes effect?

[codecov]

Codecov Report

Merging #2165 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2165      +/-   ##
==========================================
- Coverage   69.61%    69.6%   -0.01%     
==========================================
  Files         482      482              
  Lines      254580   254646      +66     
==========================================
+ Hits       177218   177254      +36     
- Misses      77362    77392      +30

Impacted Files	Coverage Δ
lib/cyclotom.gi	73.92% <0%> (-1.44%)	⬇️
src/objset.c	81.49% <0%> (-0.23%)	⬇️
src/hpc/traverse.c	95.47% <0%> (+0.02%)	⬆️
src/funcs.c	78.44% <0%> (+0.13%)	⬆️
src/hpc/threadapi.c	36.9% <0%> (+0.18%)	⬆️

[ChrisJefferson]

Commit d51d458 in #2057

[fingolfin]

@ChrisJefferson I don't understand, I see noRandom in that commit?

[fingolfin]

Ahhh, I assume f48dc21 is meant -- I commented there on a changed Random output, which I did not understand. Now I get it.

[ChrisJefferson]

I'mfairly sure I do mean d51d458.
While that commit doesn't exactly mention Random, it seems to effect it.

That can be observed by:

gap> g := CyclicGroup(IsMatrixGroup, GF(3), 3);
Group([ [ [ 0*Z(3), Z(3)^0, 0*Z(3) ], [ 0*Z(3), 0*Z(3), Z(3)^0 ], [ Z(3)^0, 0*Z(3), 0*Z(3) ] ] ])
gap> Read("tst/testrandom.g");
gap>  randomTest(g, Random);

Which will fail on that commit.

We can fix the problem by asking for NiceMonomorphism(g) before calling randomTest, or alternatively calling Random(g) at least once (which itself creates a NiceMonomorphism).

[stevelinton]

Logically the "call this a bug" option would seem to mean that we want Random, with the same random seed, to produce the same results on equal objects, the fact that the two objects in this case are the same one with different known attributes is kind of incidental.

I think this would be a very high burden to impose. An alternative would be try and document, for each category of collections, exactly what promises Random does make -- eg it could be consistent for permutation groups with the same ordered generators, but not for equal groups given by different generating sets.

I'm not wild even about this, which leads me to suggest that this test should probably just be advisory for developers -- did you lose repeatability by accident, or was it an unavoidable consequence of something else.

[fingolfin]

I agree with @stevelinton -- imposing any kind of consistency rule here will be extremely painful to implement, for a very small gain. As it is, even without this rule, all computations are still 100% deterministic, but of course there are cases were it is tricky to repeat a computation unless one can repeat the input 100% precisely. That's annoying, but still relatively uncommon and exotic, and not enough reason to limit implementors by imposing a strict rule here.

Of course in specific cases, we can and should try to make separate Random methods compatible, but even in the one that triggered this PR, I don't see how, short of removing the useful optimization of computing random elements through a nice monomorphism (or something similar), if available...

BTW, I am going to @-mention @hulpke now, just in case he has something to add?

[ChrisJefferson]

I'm happy to keep adjusting this test in future, where approriate. I certainly wouldn't want to enshrine into GAP any promises about repeatability.

At the moment it isn't even true that if you run the same GAP twice with the same inputs, you get the same outputs (as some algorithms run based on time).