Improve documentation for random sources #4438
Conversation
- document that `Random` for a list is defined only for nonempty and dense lists; the declaration for arbitrary lists is kept in order not to break code in the IO package - install `Random` methods for lists with the requirement `IsDenseList` (thus we get a "no method found" error if a non-dense list is given) - document that `Random` for two integers is defined only if the first is not larger than the second - describe the user interface for random sources in the subsection about `IsRandomSource` - added a subsection on developing new kinds of random sources - point to the user interface and the developer interface in the manual section about random sources - added a manual example for `RandomList` - added some cross-references - call `Reset` with one argument in `START_TEST` (does not change the behaviour) - document `Init` only via an index entry, since is not needed by users, removed the unary version of `Init`, as it is apparently not used - removed the `Random` method for `IsMersenneTwister` and `IsList`, because the generic method is sufficient - added a few tests for `IsMersenneTwister`; we do not promise error messages if the arguments do not fit, thus we do not need tests for this situation
ThomasBreuer
added
kind: enhancement
lib/random.gd
Outdated
| ## Note that the generic unary <Ref Oper="Reset"/> method uses the default | ||
| ## seed <C>1</C>. | ||
| ## <P/> | ||
| ## An example of an implementation as described here is given by the |
There was a problem hiding this comment.
of course the library implementations for IsGAPRandomSource and IsMersenneTwister are also examples... :-)
There was a problem hiding this comment.
The method installations for IsMersenneTwister fit.
Concerning IsGAPRandomSource, the code does not follow the recommended pattern: A Random method for a list gets installed, which relies on a default method that delegates the two-integer variant to the list variant.
| ## (pseudo-)random element of the list or collection <A>listorcoll</A>. | ||
| ## (pseudo-)random element of the dense, nonempty list or nonempty | ||
| ## collection <A>listorcoll</A>. | ||
| ## The behaviour for non-dense or empty lists, and for empty collections |
There was a problem hiding this comment.
Just as an aside, empty collections are a rare thing in GAP... Actually I don't know any example other than empty magmas, as in issue #4424 (CC: @wilfwilson). My guess is that there is quite a bit more code which does in fact not expect collections to be empty (just like we used to have quite a lot of code that didn't work right for groups with GeneratorsOfGroup(G) = [])
There was a problem hiding this comment.
As I mentioned in a comment for #4424, there is some support for empty semigroups.
| @@ -201,18 +203,18 @@ InstallMethod(Reset, [IsMersenneTwister, IsObject], function(rs, seed) | |||
| return old; | |||
| end); | |||
|
|
|||
| InstallMethod(Random, [IsMersenneTwister, IsList], function(rs, list) | |||
| return list[Random(rs, 1, Length(list))]; | |||
| end); | |||
There was a problem hiding this comment.
@ChrisJefferson I have a vague memory that when you refactored this code last, we had to keep some methods (for ... something...?) instead of generic methods due to some issues that I forgot about... I am hoping this is not relevant here (in particular, I believe the tests for Random that you wrote would hopefully catch any such issues), but perhaps you could have a quick look at the changes in this PR just in case you remember this better than I do... :-)
There was a problem hiding this comment.
At some point when I worked on this I tried to make sure that GAP would produce the same sequence of random numbers on different versions -- now I think that wasn't worth the effort, as for most types we don't make that promise anyway. In some cases I found infinite loops in packages if I removed the wrong function, but if this passes all package tests then I think it's OK.
There was a problem hiding this comment.
I see the situation as follows.
We have generic Random methods for the cases [ IsRandomSource, IsList and ... ] and [ IsRandomSource, IsInt, IsInt ].
They delegate to each other, with the effect that for specific kinds of random sources, one has to install only one such method. (If one does not install any Random method for a new kind of random sources, one runs into infinite loops, thus the generic methods are to some extent dangerous. However, we do not support "generic random sources".)
The Random methods for different specific kinds of random sources do not compete with each other, thus only the specific method (if available) and the generic method are applicable.
In the case of IsMersenneTwister, the specific method for IsList which I propose to remove does the same as the generic method for this case, thus the removal should not change anything at runtime, in particular the sequence of random numbers should not be affected.
fingolfin
changed the title
document that
Randomfor a list is defined only for nonempty anddense lists; the declaration for arbitrary lists is kept in order
not to break code in the IO package
install
Randommethods for lists with the requirementIsDenseList(thus we get a "no method found" error if a non-dense list is given)
document that
Randomfor two integers is defined only if the firstis not larger than the second
describe the user interface for random sources in the subsection
about
IsRandomSourceadded a subsection on developing new kinds of random sources
point to the user interface and the developer interface in the
manual section about random sources
added a manual example for
RandomListadded some cross-references
call
Resetwith one argument inSTART_TEST(does not change the behaviour)
document
Initonly via an index entry, since is not needed by users,removed the unary version of
Init, as it is apparently not usedremoved the
Randommethod forIsMersenneTwisterandIsList,because the generic method is sufficient
fixed
Random( IsMersenneTwister, 1, 0 )and added a few tests;in general, we do not promise error messages if the arguments do not fit,
thus we do not need tests for this situation
(resolves #4379 and #4403;
replaces #4415: in a discussion with @fingolfin, we decided better not to extend the documentation of
Randomto non-dense lists)