Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow optionally passing a random source to RandomList, and to Random for collections #2204

Merged
merged 3 commits into from
Mar 20, 2018

Conversation

ChrisJefferson
Copy link
Contributor

The aim of this PR is to let Random for a collection take a random source. This required adding a random source also to RandomList, and do a little rearranging.

@ChrisJefferson
Copy link
Contributor Author

This in case anyone wonders, I kept RandomList as a function rather than making it a Method because I wanted to keep it fast and simple (and to be able to define it in the same place, early).

@fingolfin
Copy link
Member

If RandomList was an operation (I assume you meant that?), that would indeed be a major speed penalty, as method dispatch overhead for list arguments is the bit that's super-expensive. Might be worth adding a comment to RandomList indicating that it must be kept as a function (just like I added comments for List etc.:

    # handle built-in lists directly, to avoid method dispatch overhead

lib/coll.gd Outdated
##
## <Description>
## <Index>random seed</Index>
## For a dense list <A>list</A>,
## <Ref Func="RandomList"/> returns a (pseudo-)random element with equal
## distribution.
## <P/>
## This function uses the <Ref Var="GlobalMersenneTwister"/> to produce the
## The random generator <A>rs</A> is used to choose a random number.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generator -> source

##
## <Description>
## <Index>random seed</Index>
## For a dense list <A>list</A>,
## <Ref Func="RandomList"/> returns a (pseudo-)random element with equal
## distribution.
## <P/>
## This function uses the <Ref Var="GlobalMersenneTwister"/> to produce the
## The random generator <A>rs</A> is used to choose a random number.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generator -> source

lib/coll.gi Outdated
local len;
len := Length(args);
if len = 1 then
return (args[1])[Random(GlobalMersenneTwister, 1, Length(args[1]))];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this difficult to read. May I suggest:

InstallGlobalFunction( RandomList, function(args...)
local rs, list, len;
len := Length(args);
if len = 1 then
  rs := GlobalMersenneTwister;
  list := args[1];
elif len = 2 then
  rs := args[1]
  list := args[2];
else
  Error(...);
fi;
return list[Random(rs, 1, Length(list))];
end );

lib/random.gd Outdated
{rs,C} -> RandomList(rs, Enumerator( C ) ) );

RedispatchOnCondition(Random,true,[IsCollection],[IsFinite],0);
RedispatchOnCondition(Random,true,[IsRandomSource, IsCollection],[IsFinite],0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I am not so happy about moving these collection specific functions out of coll.gi. Here is a possible alternative, which allows using InstallMethodWithRandomSource inside coll.gi:

diff --git a/lib/coll.gi b/lib/coll.gi
index 264a63de0..3f63919b2 100644
--- a/lib/coll.gi
+++ b/lib/coll.gi
@@ -246,11 +246,6 @@ InstallMethod( RepresentativeSmallest,
 ##  an enumerator of <C> and selects a random element of this list using the
 ##  function `RandomList', which is a pseudo random number generator.
 ##
-if IsHPCGAP then
-    MakeThreadLocal( "GlobalMersenneTwister" );
-else
-    DeclareGlobalVariable( "GlobalMersenneTwister" );
-fi;
 InstallGlobalFunction( RandomList, function(list)
   return list[Random(GlobalMersenneTwister, 1, Length(list))];
 end );
diff --git a/lib/random.gd b/lib/random.gd
index c582a930c..763c31482 100644
--- a/lib/random.gd
+++ b/lib/random.gd
@@ -39,7 +39,7 @@
 ##  </ManSection>
 ##  <#/GAPDoc>
 ##
-BindGlobal( "RandomSourcesFamily", NewFamily( "RandomSourcesFamily" ) );    
+BIND_GLOBAL( "RandomSourcesFamily", NewFamily( "RandomSourcesFamily" ) );
 DeclareCategory( "IsRandomSource", IsComponentObjectRep );
 
 #############################################################################
@@ -105,6 +105,14 @@ DeclareOperation( "Random", [IsRandomSource, IsInt, IsInt] );
 ##  </ManSection>
 ##  <#/GAPDoc>
 ##
+
+if IsHPCGAP then
+    MakeThreadLocal( "GlobalMersenneTwister" );
+else
+    DeclareGlobalVariable( "GlobalMersenneTwister" );
+fi;
+
+
 (function()
     local func;
     func := function(installType)
diff --git a/lib/read1.g b/lib/read1.g
index b8d21f9c8..a360f1c4b 100644
--- a/lib/read1.g
+++ b/lib/read1.g
@@ -36,6 +36,8 @@ ReadLib( "set.gd"      );
 
 ReadLib( "record.gd"   );
 
+ReadLib( "random.gd"   );
+
 ReadLib( "cache.gi"    );
 ReadLib( "coll.gi"     );
 
@@ -70,8 +72,6 @@ ReadLib( "info.gi"     );
 ReadLib( "assert.gi"   );
 ReadLib( "global.gi"   );
 
-ReadLib( "random.gd"   );
-
 ReadLib( "options.gd"  );
 ReadLib( "options.gi"  );
 

To me, it anyway nicer to declare GlobalMersenneTwister inside random.gd instead of coll.gi -- this matches GlobalRandomSource. The patch above is not quite what I'd do, though: I'd rather move the definition of GlobalMersenneTwister where it belongs, i.e. next to the def of GlobalRandomSource (where we actually already have commented out definitions for GlobalMersenneTwister!). But this means the def for InstallMethodWithRandomSource has to be moved downwards, which is simple, but leads to a huge diff).

I can also make a PR with these changes now...

@codecov
Copy link

codecov bot commented Feb 23, 2018

Codecov Report

Merging #2204 into master will decrease coverage by 0.03%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #2204      +/-   ##
==========================================
- Coverage   70.38%   70.35%   -0.04%     
==========================================
  Files         481      480       -1     
  Lines      253228   252217    -1011     
==========================================
- Hits       178230   177437     -793     
+ Misses      74998    74780     -218
Impacted Files Coverage Δ
hpcgap/lib/coll.gd 91.3% <ø> (ø) ⬆️
lib/coll.gd 93.52% <ø> (ø) ⬆️
lib/random.gi 83.91% <100%> (+5.74%) ⬆️
lib/coll.gi 94.62% <90.9%> (-0.04%) ⬇️
src/gapstate.h 71.42% <0%> (-28.58%) ⬇️
lib/files.gi 27.04% <0%> (-17.63%) ⬇️
src/system.c 60.57% <0%> (-5.43%) ⬇️
lib/files.gd 56.25% <0%> (-5.21%) ⬇️
src/streams.c 54.53% <0%> (-3.87%) ⬇️
src/scanner.c 87.67% <0%> (-2.63%) ⬇️
... and 47 more

@ChrisJefferson ChrisJefferson added the do not review PRs which are not yet ready for a proper external review (e.g. only submitted for test results) label Feb 23, 2018
@ChrisJefferson ChrisJefferson force-pushed the randomList branch 2 times, most recently from 468c7b9 to 5164058 Compare March 1, 2018 18:41
@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Mar 1, 2018

Assuming the tests pass, I'm now happy with this. I use -SUM_FLAGS to make sure the random collection overload is put below everything else.

@ChrisJefferson ChrisJefferson removed the do not review PRs which are not yet ready for a proper external review (e.g. only submitted for test results) label Mar 1, 2018
lib/coll.gi Outdated
# In particular, we want Random(SomeRandomSource, IsList) to come higher
# for lists, to avoid an infinite loop.
InstallMethodWithRandomSource( Random, "for a (finite) collection",
[ IsCollection and IsFinite ], -SUM_FLAGS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid extra ranking of methods whenever possible. And the -SUM_FLAGS does not seem needed here. E.g., in current master branch:

gap> DeclareCategory("IsDice", IsComponentObjectRep and IsDomain);
gap> dice := Objectify(NewType(NewFamily(""), IsDice), rec());
<object>
gap> InstallMethod(Enumerator, [IsDice], d-> [1..6]);
gap> 
gap> InstallMethod(Random, ["IsRandomSource", "IsCollection and IsFinite"],
> function(rs, coll) return Random(Enumerator(coll)); end);
gap> Random(dice);
2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, there is a small bug here, sorry.

This shows the issue I'm trying to fix. Given the following code, then Random(GlobalRandomSource,[1..10]) enters an infinite loop, because this newly installed method ranks more highly than the InstallMethod(Random, [IsGAPRandomSource, IsList],... in random.gi.

InstallMethod( Random, "for a source and a (finite) collection",
    [ IsRandomSource, IsCollection and IsFinite ],
    {rs,C} -> RandomList(rs, Enumerator( C ) ) );


MakeReadWriteGlobal("RandomList");
RandomList := function(args...)
  local len, source, list;
  len := Length(args);
  if len = 1 then
    source := GlobalMersenneTwister;
    list := args[1];
  elif len = 2 then
    source := args[1];
    list := args[2];
  else
    Error(" Usage: RandomList([rs], list))");
  fi;

  return list[Random(source, 1, Length(list))];
end;

@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Mar 2, 2018

I am copying this here so it doesn't get lost:

This shows the issue I'm trying to fix. Given the following code, then Random(GlobalRandomSource,[1..10]) enters an infinite loop, because this newly installed method ranks more highly than the InstallMethod(Random, [IsGAPRandomSource, IsList],... in random.gi.

InstallMethod( Random, "for a source and a (finite) collection",
    [ IsRandomSource, IsCollection and IsFinite ],
    {rs,C} -> RandomList(rs, Enumerator( C ) ) );


MakeReadWriteGlobal("RandomList");
RandomList := function(args...)
  local len, source, list;
  len := Length(args);
  if len = 1 then
    source := GlobalMersenneTwister;
    list := args[1];
  elif len = 2 then
    source := args[1];
    list := args[2];
  else
    Error(" Usage: RandomList([rs], list))");
  fi;

  return list[Random(source, 1, Length(list))];
end;

lib/coll.gi Outdated
InstallGlobalFunction( RandomList, function(list)
return list[Random(GlobalMersenneTwister, 1, Length(list))];

# RandomList is not a Method to avoid the (often expensive) cost of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method -> method (only change this if you end up having to update this PR anyway; no need to trigger a full rebuild over such minor thing ;-)/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, strictly speaking, I guess this should be "is not an operation", though?

lib/random.gi Outdated
@@ -311,3 +311,10 @@ end );
InstallGlobalFunction("InstallMethodWithRandomSource", func(InstallMethod));
InstallGlobalFunction("InstallOtherMethodWithRandomSource", func(InstallOtherMethod));
end)();

# Use -SUM_FLAGS, as this should always be the last choice method.
# In particular, we want Random(SomeRandomSource, IsList) to come higher
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to come higher" -> "to be ranked higher" ?

tst/testrandom.g Outdated
fi;
# Perform a variety of tests on Random generators.
# filter: The type of random source we are testing
# globalobj: The name of the global instance of this iterator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"this iterator" ? Huh?

Also, why is this named globalobj, and in how far does the object passed in have to be "global" ? If I create such an instance on the fly, and also create gmethod on the fly, using this, it would be fine, too?

How about:

globalrs: an instance of a random source in (typically a global instance thereof, like , to be used by gmethod.

tst/testrandom.g Outdated
else
checkmethod := checkin[1];
fi;
# Perform a variety of tests on Random generators.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand "Random generators" here! First I thought you meant "random source", but that doesn't make sense. My next guess was "methods for the Random method", but that also doesn't seem to make sense, as the test are not restricted to Random.

Right now I think you might mean: "Perform a variety of tests with a given function that is expected to produce/generate random output, and which should allow passing in a random source as optional first argument."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep using random generator, when I mean source. Will generally clean up.

tst/testrandom.g Outdated
# globalobj: The name of the global instance of this iterator
# gmethod: A single argument method which takes elements of the collection
# using globalobj (This exists to let us test single-argument Random)
# method: Two argument random method, which takes a random source and collection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the name gmethod? Also, why "method" -- these two can be any function, can't they? And we frequently pass in operations, not methods...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method is just a bad choice of name, I tend to use method to refer to "any function", when it has a special meaning in GAP.

tst/testrandom.g Outdated
# checkmethod: a way of checking if a value is a member of collection (usually \in)

randomTestInner := function(filter, globalobj, gmethod, method, collection, checkmethod)
local test1, test2, test3, test4, test5, test6, localgen;

# We do a single call first, to deal with calling Random causing extra attributes
# of 'collection' to be set, changing the dispatch
method(collection);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this perhaps be gmethod? Otherwise, the documentation comment is wrong, as here method is called with a single argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definately wrong

tst/testrandom.g Outdated
# checkmethod: a way of checking if a value is a member of collection (usually \in)

randomTestInner := function(filter, globalobj, gmethod, method, collection, checkmethod)
local test1, test2, test3, test4, test5, test6, localgen;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So localgen is the sibling to globalobj, and also a randomsource... perhaps call them rs and gobal_rs then?

tst/testrandom.g Outdated
fi;

randomTestInner(IsMersenneTwister, GlobalMersenneTwister, x -> method(x), method, collection, checkmethod);
randomTestInner(IsGAPRandomSource, GlobalRandomSource, x -> method(GlobalRandomSource, x), method, collection, checkmethod);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we once x -> method(x) and once x -> method(GlobalRandomSource, x)? Aha, I assume there is an implicit assumption is that if no RS is specified, then GlobalMersenneTwister is used? The manual only says this (emphasis mine):

Most methods for Random in the GAP library use the GlobalMersenneTwister as random source.

I am happy to enforce this here in the testing code, but I think this should be stated explicitly, at least in a comment right here, e.g. like this:

If no random source is specified, functions producing randomness are expected to default to using

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should possibly strengthen the documentation, to "All methods", I believe the only method which uses anything else is the undocumented RANDOM_LIST.

tst/testrandom.g Outdated
# A special test for collections of size 1
randomTestForSizeOneCollection := function(collection, method)
# Here we can't check different seeds produce different answers
# We do check that the random source is not used, for efficency.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While that seems sensible, is this documented anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it seemed to be true in practice, so I decided to test it. I'm not positive I'd want to enforce it, but if it was ever false I'd certainly want to investigate why, as I imagine we'd be doing pointless work.

tst/testrandom.g Outdated

if intlist1 <> intlist2 then
Print("Random read from local gen affected global gen: ", collection);
fi;
end;;

randomTestForSizeOneCollection := function(collection, method, checkin...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I wonder: is there a strong reason to have randomTestForSizeOneCollection be a separate function? Couldn't randomTest choose between calling randomTestInner and randomTestForSizeOneCollectionInner based on the collection size? (Even better, it could use IsTrivial on the collection, which means "size = 1" but may work even on collections which don't know their precise size)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should do that.

@ChrisJefferson
Copy link
Contributor Author

I believe this is now cleaned up and all issues fixed.

@frankluebeck
Copy link
Member

@ChrisJefferson Thanks for the example. This is not a specific problem of Random, but occurs for all operations when you want to install a method that should be used for finite collections which are not lists.

Still, an inflationary -SUM_FLAGS should be avoided, a -8 or so also would do the job.

But I would remove the -SUM_FLAGS here completely and instead push up the method for [IsGlobalRandomSource, IsList] by 100, that is what is done for the other cases.

@fingolfin
Copy link
Member

@frankluebeck could you elaborate why you would avoid ˋ-SUM_FLAGSˋ, resp. why the alternatives are "better"? (Are they? Why?) I.e., other than "people in the past did it that way". Right now this just seems like an equivalent alternative, not worth making @ChrisJefferson jump through hoops. Some factial justification would be very helpful, and motivating.

What I don't like about an hardcoded +100 for the Random method for IsList is that I am concerned that it might shadow some other method somebody might install for special kinds of lists. E.g. I might imstall one for ranges, now I need to know I have to give it a +100.

@ChrisJefferson
Copy link
Contributor Author

The problem with pushing up Random(IsGlobalRandomSource,IsList) is that I don't want it to get too high, else it might get above Random(IsRandomSource,IsX) for some X which implements IsList (I haven't checked everything we implement Random for). Also, we would have to do the same with every current and future added random source (and any random sources anyone has already implemented in their own code).

Of course the other option is my other plan, which was to delete Random(IsGlobalRandomSource,IsList) and replace it with Random(IsGlobalRandomSource,IsInt,IsInt), which removes the issue (and also requires changes of course).

PS sorry there is several random changes at the same time. When I started I did not expect this to get so complicated.

tst/testrandom.g Outdated
#
# Test that 'Random(rs,C)' only uses 'rs', and no other source of random
#
# Test Random, PseudoRandom and RandomList
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, ˋPseudoRandomˋ does not yet allow a random source, and in fact for some methods (e.g. for groups) it may not be possible to reset the state of a random source and then get the same set of pseudo random elements, because there is additional mixing state in the group

tst/testrandom.g Outdated
#
# Where there is a global instance of a random generator
# (GlobalMersenneTwister and GlobalRandomSource), they produce the same
# sequence of answers as an new instance of the same random source.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An new -> a new

tst/testrandom.g Outdated
# global_randfunc(C): A one argument function which is equivalent to
# {x} -> rand(global_rs, x). This lets us check 'Random(C)' and
# 'Random(GlobalMersenneTwister,C)' produce the same answer when testing
# GlobalMersenneTwister. For other random generators, this can just
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generator -> source (and perhaps grep for other occurrences?)

tst/testrandom.g Outdated
Init(GlobalMersenneTwister, 6);
test1 := List([1..1000], x -> method(collection));
Init(global_rs, 6);
test1 := List([1..1000], x -> global_randfunc(collection));
# test2 should = test1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confused me at first, maybe spell out "equal"?

# so this should be the same as intlist1
intlist2 := List([1..1000], x -> Random([1..10]));
intlist2 := List([1..10], x -> global_randfunc([1..1000]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just query the state before and after, and compare it? Make an immutable copy to make sure it's not been modified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like testing the actual sequence. However, it might be nice to also test the states are equal as well!

tst/testrandom.g Outdated
GlobalRandomSource, x -> randfunc(GlobalRandomSource, x), randfunc,
collection, checkin);
fi;
end;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a trailing newline?

tst/testrandom.g Outdated
GlobalRandomSource, x -> randfunc(GlobalRandomSource, x), randfunc,
collection, checkin);
else
randomTestInner(IsMersenneTwister,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could avoid some code duplication by adding a local var which you set to either ˋrandomTestInnerˋ or the orher func.

@frankluebeck
Copy link
Member

Ad why to avoid SUM_FLAGS, actually, why to avoid any extra ranking in method installation:
In early times of GAP4 there were many methods installed with SUM_FLAGS, then 2SUM_FLAGS,
3
SUM_FLAGS+4 and so on because several people thought that their method is definitely the "best ever". Many further methods where installed with other rank shifts. By the time we found and agreed that method ranking is easier to understand and maintain if the filters are ranked such that rank shifts in method installations can be avoided. And +/-SUM_FLAGS should only be used if a method is definitely the most/least significant one. More generally I would try to avoid rank shifts for methods and if this seems difficult, then use a mild shift.

Now to the specific case: this method is not one of the last resort type (like methods to print certain error messages or to avoid "no method found" errors) and so not a case to use -SUM_FLAGS.

The case of installing a method for IsFinite and IsCollection which should not be called for IsList (which unfortunately implies IsCollection) is one where I don't see how to do it without a rank shift. Checking the filter ranks a shift of -3 would be enough. If you want to leave place to maybe put another method in between, then -8 would be good.

Now to the methods for IsList with shift +100: I don't know why +100 was chosen and have not claimed that this is a better solution. But I have suggested that all cases of random sources should be dealt with in the same way. Maintaining the methods for Random does not become easier (and that is the goal of this pull request, isn't it?) when the base methods, say for IsGAPRandomSource and IsMersenneTwister, have completely different ranks.

If anyone thinks that I'm only writing comments to make poor "@ChrisJefferson jump through hoops" feel free to ignore them.

@ChrisJefferson
Copy link
Contributor Author

@frankluebeck : I don't mind discussing this for a while, this is a sensitive, very old and very well-used bit of code in GAP!

Stepping back for a moment, the short-term aim of this PR is to allow calling Random(RS,C) for any random source RS and collection C and RandomList(RS,L) for any random source RS and list L.

My final long term aim is to make it easy for users to add new implementations of Random for new collections which work with any random source, and also new implementations of Random for new random sources which work with any collection. I'd like it (as far as possible) to not require us to be careful about adjusting priorities when those things get done, particularly because they might be done in packages.

This requires moving the function to random.gi, as we want to use
InstallMethodWithRandomSource.
@ChrisJefferson
Copy link
Contributor Author

i believe all issues in this PR are now fixed.

# This method must rank below Random(SomeRandomSource, IsList)
# for any random source SomeRandomSource, to avoid an infinite loop.
InstallMethodWithRandomSource( Random, "for a random source and a (finite) collection",
[ IsRandomSource, IsCollection and IsFinite ], -8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for being nitpicky, but: now that value -8 seems rather arbitrary, and I'd really prefer if there was a comment here (resp. before the InstallMethod... invocation) that explains why it has the value; i.e. to push it below the method for IsList ?!?

I wonder if this value could/should instead be computed? Like, using RankFilter(IsList) - RankFilter(IsCollection and IsFinite) - 6 instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, shouldn't those methods in random.gi use IsList and IsFinite instead of IsList anyway? They require finite lists, after all (if Length(list) returned infinity, they'd have a problem). Wouldn't that also resolve the rank problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IsList and IsFinite would fix the generators provided in the library, but still cause an infinite loop in any other random generator (like the one in IO).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the comment already explain? We need to rank below Random(SomeRandomSource, IsList) for any SomeRandomSource.

@fingolfin fingolfin merged commit 4dedfee into gap-system:master Mar 20, 2018
@fingolfin fingolfin changed the title Random list Allow optionally passing a random source to RandomList Mar 28, 2018
@fingolfin fingolfin changed the title Allow optionally passing a random source to RandomList Allow optionally passing a random source to RandomList, and to Random for collections Mar 28, 2018
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: added PRs introducing changes that have since been mentioned in the release notes labels Mar 28, 2018
@ChrisJefferson ChrisJefferson deleted the randomList branch March 28, 2018 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants