Help Random methods to use RandomSource

[ChrisJefferson]

The idea behind this patch is that we want (long term) for all implementations of Random to take an optional RandomSource.

Given an implementation of Random which takes a RandomSource, it's easy to wrap it and make a version which doesn't take a random source. This is what InstallRandomMethod does.

I show an example of using this on Integers. If this is happily accepted, then hopefully I (or other people!) will convert all InstallMethod for Random to use InstallRandomMethod. This will allow users to control random numbers better, and avoid code duplication.

[frankluebeck]

Wouldn't the following fallback method do the trick?

InstallOtherMethod(Random, [IsObject], function(x) 
  return Random(GlobalMersenneTwister, x); 
end);

For the few cases where the double call of Random may be a performance problem one can still install two methods.

(BTW: InstallRandomMethod is not a good name, maybe InstallMethodForRandom? But I would prefer not to create a very specialized mechanism for this operation.)

[ChrisJefferson]

Sorry, I should have explained why this is necessary. The problem isn't performance, it's method selection.

If I could change every InstallMethod for Random in the GAP library, and every package, in one go, then I could do what you suggest (but I don't think that's reasonable).

However, I convert Random for IsSymmetricGroup to two arguments, but not IsPermGroup then Random(SymmetricGroup(5)) would choose IsPermGroup before trying the fallback method.

I considered extending dispatch to allow it to choose between the one and two argument versions of Random, but that was much more complicated, and I wasn't sure would ever have any other purpose.

Once every installed Random method uses InstallMethodForRandom, then we could change InstallMethodForRandom to just be InstallMethod again, add the fallback method, and tell people to never add 1-argument Random methods.

[fingolfin]

So... couldn't we then add a method like Frank suggested, but with rank SUM_FLAGS or so? Then this would override all single argument variants of the method, and redirect them to the two argument version.

Of course that would only work if all packages actually provide both 1 and 2 arg versions of Random -- it shouldn't be hard to verify that, though, and if any packages doesn't do it, we can ask its maintainesr to fix that first.

[fingolfin]

Hmm... I just checked, and actually virtually no packages provides Random methods which take a RandomSouce (the io package is a notable exception). In addition, a few provide two argument methods for Random where the first argument isn't a RandomSource.

So, since we have to prod almost all package authors anyway, I wonder whether it really help to have InstallMethodForRandom? After all, all it does is safe me adding 3 lines (or so) of very simple code -- the "hard" (well, not very) part is to change the existing Random methods to take a random source as first argument; adding the single param variant is routine.

This also has the advantage that package authors don't need to add a dependency on GAP >= 4.x.y, where 4.x.y is the version which introduced InstallMethodForRandom. This in turn makes it easier to roll out an update. So for "my" packages, I would actually prefer not to use InstallMethodForRandom.

Regardless of whether we merge the PR or not, I think we will have to invest some effort (and careful explain things) if we want package authors to change their packages.

[ChrisJefferson]

I'd be happy to take this PR away, and just suggest we work our way through packages and the library, converting 1 arg Randoms to two arg Randoms, and adding the 2-arg -> 1-arg specialisation by hand that InstallMethodForRandom would add.

The main thing I want is 2-arg Randoms, without having a "break the world" day (because that never works well).

[frankluebeck]

Ok, if the goal is to have versions of all methods for Random with a random source argument then I don't see how this proposed function would help. Most, if not all, methods delegate to Random for lists or two integers. The main work will be that the code of these methods must be changed such that the internal Random calls use the given random source (and not that another two lines for the one argument version are needed - which could also be done with a fallback method as mentioned above). I don't see why this change could not be made by an by. (There will be only few cases where several methods are applicable.)

[fingolfin]

I discussed this with @ChrisJefferson some time ago, and while initially sceptical, I now think something along this vein could be benficial. I'll try to explain.

First off, this is based on the assumption that it is desirable for us to adapt as many as possible (ideally, all) Random (and PseudoRandom, and possible others) methods to allow specifying a random source. At the same time, of course backwards compatibility should be preserved in that the random source should be optional.

If one agrees with this, then now the question is how to get there. Right now, virtually all Random (etc.) methods don't take a random source. The first step then should be to change the library to provide methods taking a random source for all our Random (etc.) methods. This will then later on enable package authors to change their Random methods (which often invoke Random methods from the GAP library, thus need to be able to pass random sources to those).

Now, as Frank correctly points out, this can right already be done by modifying the existing Random method to take (and use) a random source (which is most of the work required), and then add a second method with the old signature (i.e. not taking a random source) which passes GlobalMersenneTwister to the proper two-arg methods.

While doing this is indeed relatively easy, there are two caveats:

1. This is a stupid, repetetive task, and while trivial, it is still work, and still quite easy to mess it up by accident for humans.
2. Once "everything" is converted (on "day X"), we might want to remove all those wrappers, and replace them by a single "master wrapper" which simply redispatches.

If we introduce an InstallMethodForRandom, we solve both of these: We reduce the repetition and potential for mistakes, and on day X, we can simply change it to not install all those custom methods without a random source.

In order to make this generic, I think InstallMethodForRandom should actually have the exact same signature as InstallMethod (so that it can be used on e.g. PseudoRandom, too). Bonus points if it also supported installing methods with more than two args -- several packages extend Random to take multiple arguments, and these would also benefit from having an optional random source as first arguments. Perhaps there should also be an InstallOtherMethodForRandom...

After day X, InstallMethodForRandom would simply become a synonym for InstallMethod, and then packages could start replacing the former again with the latter.

[fingolfin]

Another minor thing: Perhaps InstallMethodForRandom could check whether the first filter in the arg filter list implies IsRandomSource, as a kind of sanity check that prevents people from silly mistakes (e.g. changing InstallMethod(Random, [IsFooBar], ... to InstallMethodForRandom(Random, [IsFooBar], ... , i.e. without making any adjustments.

[markuspf]

What is the status of this PR?

[ChrisJefferson]

I've now adjusted this so that the new method, InstallMethodForGlobalRandomGen looks exactly the same as InstallMethod.

Hopefully this can now get merged, at which point I'll start translating other methods.

I haven't done InstallOtherMethod yet, as none of them are in the standard library for Random. We should investigate that later.

[codecov-io]

Current coverage is 50.55% (diff: 59.63%)

Merging #810 into master will increase coverage by 0.07%

@@             master       #810   diff @@
==========================================
  Files           424        426     +2   
  Lines        223042     223146   +104   
  Methods        3430       3430          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         112594     112804   +210   
+ Misses       110448     110342   -106   
  Partials          0          0          

Diff Coverage	File Path
•••• 45%	new tst/testrandom.g
••••••• 77%	lib/random.gd
•••••••••• 100%	lib/padics.gi
•••••••••• 100%	lib/list.gi
•••••••••• 100%	lib/integer.gi

Powered by Codecov. Last update 7c91a62...c2c478d

[fingolfin]

Great work, thanks!

[fingolfin]

Doesn't that mean the function will appear in the manual right after InstallMethod and InstallOtherMethod? Do we want that? There are some arguments for it, I guess, but it would also make sense to move this near the documentation for random sources... Alas, see also issue #1057

[fingolfin]

This looks plausible; does it fix any actual problem? If so, perhaps this should be documented in the PR text and/or covered by a test case? (Or perhaps it already is covered by a test case?)

[ChrisJefferson]

It is covered. This just lines up two-argument random with one argument random, which was already DeclareOperation( "Random", [ IsListOrCollection ] );. It's needed to take random elements from lists which aren't collection.

[fingolfin]

It doesn't work quite the "same", does it? Perhaps "which takes the same arguments as" ?

Also, change the comma at the end to a period, or else rephrase the next line (at the very least, make it start lowercase).

[fingolfin]

I guess StartsWith is not available here?

Hmm... looking at lib/read*.g, it seems we used to read it later, but it was moved to read1.g. Testing this, the only reason for that seems to be that matobj2.gd references random sources... So we can fix this (and you can start using things like BindGlobal and StartsWith here) quite easily. This patch does it for me:

diff --git a/lib/read1.g b/lib/read1.g
index 185f74763..3843fdbfd 100644
--- a/lib/read1.g
+++ b/lib/read1.g
@@ -67,8 +67,6 @@ ReadLib( "info.gi"     );
 ReadLib( "assert.gi"   );
 ReadLib( "global.gi"   );

-ReadLib( "random.gd"   );
-
 ReadLib( "options.gd"  );
 ReadLib( "options.gi"  );

diff --git a/lib/read3.g b/lib/read3.g
index 51a34f289..40f2730c3 100644
--- a/lib/read3.g
+++ b/lib/read3.g
@@ -82,9 +82,6 @@ ReadLib( "unknown.gd"  );
 ReadLib( "word.gd"     );
 ReadLib( "wordass.gd"  );

-ReadLib( "matobj2.gd"  );
-ReadLib( "matobjplist.gd" );
-
 # files dealing with rewriting systems
 ReadLib( "rws.gd"      );
 ReadLib( "rwspcclt.gd" );
@@ -174,9 +171,13 @@ ReadLib( "integer.gi"  ); # needed for CoefficientsQadic
 ReadLib( "list.gi"     ); # was too early
 ReadLib( "set.gi"      );
 ReadLib( "wpobj.gi"    );
-##  ReadLib( "random.gd"   );
-##  ReadLib( "random.gi"   );
-##  ReadLib( "random.g"    );
+
+# random sources
+ReadLib( "random.gd"   );
+ReadLib( "random.gi"   );
+
+ReadLib( "matobj2.gd"  );
+ReadLib( "matobjplist.gd" );

 # files dealing with nice monomorphism
 # grpnice uses some family predicates, so fampred.g must be known
@@ -249,6 +250,3 @@ ReadLib("gasman.gd");

 # files dealing with function calling
 ReadLib("function.gd");
-
-# random sources
-ReadLib("random.gi");

[fingolfin]

Please end the file with a newline.

[fingolfin]

effected -> affected

[ChrisJefferson]

@fingolfin : I have fixed all comments, except:

• I haven't rearranged where the help page lives. I agree a general Random section should be added, and then this new method moved there.

• I tried using StartsWith, but actually it didn't work, because I needed string.gi also moving early, which turned out to be difficult. I tried moving the method to random.gi and moving that later, but actually we need this method before we declare any random methods, so it must be early itself.

[fingolfin]

@ChrisJefferson Thanks! I'll take another look

[fingolfin]

Note: travis will fails due to the broken log2int on master. So I stopped the build for this PR for now. You can either rebase it on master just before the bad build, or wait till the fix gets in and rebase it again... of course, then the 32bit build will still be broken... Hrmpf

[fingolfin]

Typo: soure -> source

[fingolfin]

Perhaps overloads -> methods? Overload is more a C++ term, unusual for GAP.

[fingolfin]

I don't understand that name. What is a "GlobalRandomGen"? Perhaps this refers to the fact that an alternate method using a global random source is installed? Then it should perhaps be InstallMethodWithGlobalRandomSource ?
Or perhaps just InstallMethodWithRandomSource (indicating it can be used to install any method whose first argument is a random source).

I don't want to bikeshed, but I really don't understand the current name.

[fingolfin]

it's -> its

[fingolfin]

"Firstly" sounds quite formal to me. Indeed, see also http://dictionary.cambridge.org/grammar/british-grammar/first-firstly-or-at-first

Perhaps just "first" ?

[fingolfin]

Hrm... that only "almost" explains what's going on. I think we should either give a high-level description, or a full explanation, or perhaps both. How about this:

"This function then installs two methods: first it calls InstallMethod with unchanged arguments. Then it calls InstallMethod a second time to install anothr method which lacks the initial random source argument; this additional method simply invokes the original method, with GlobalRandomSource added as first argument."

[fingolfin]

Ahh, so this actually only works if the mtehod takes exactly one argument in addition to the random source. But there are some Random methods which take 1+2 = 3 argument.

If the user tries to use InstallMethodWithGlobalRandomGen to set one of those up, the mistake will not be immediately visible -- only if you try to use Random without a source. So I would suggest we check the number of arguments. In addition, we could also verify that NumberArgumentsFunction(func) and Length(args[filterpos]) coincide.

Even nicer would of course be if we could support arbitrary numbers of arguments :-). But I am not going to hold this PR hostage over this :-).

[ChrisJefferson]

Changes made -- also added support for InstallOtherMethod, and changed the only use of InstallOtherMethod in the standard library.

[fingolfin]

@frankluebeck do you still haveobjections to this PR in its current form?

Otherwise, we will merge it soon... and I suggest that at the same time we create an issue to remind us to convert more code to use this (I have not checked whether there are more Random methods in the library whoch might benefit from this, but in packages those exist for sure... though there we likely will want to wait till 4.9 has been released...

[ChrisJefferson]

There are lots of methods in the library that can use this. As a brief search:

AdditiveCoset, DoubleCoset, RightCoset, hashtable, AlgebraicExtension, ConjugacyClasses, finite prime fields, pseudorandom groups with product replacement, and I've only got about 1/2 way through the library :)