Permutations from Random Sources

[stevelinton]

Allows Random( <random-source>, SymmetricGroup(<n>)) and the like.
Breaks pperm and trans tests due to changes to number of calls to the global random source, or something. The best solution would be RandomPartialPerm and similar to accept a Random Source and then use one in testing.

[frankluebeck]

Most of the time in FLOYDS_ALGORITHM is spent on the computation of (many small) random numbers. The method selection overhead is not negligible. This pull request does not only make this function more flexible (it can be used with any random source) but also about 20% faster. This is because the method that delegates Random([i..d]) to Random(GlobalMersenneTwister, [i..d]) is skipped.

We could get another 20% of speedup be skipping method selection completely:

function(rs, deg, even)
    local  rs, rnd, sgn, i, k, tmp;
    rnd := [1..deg];
    sgn := 1;
    rs := rs!.state;
    for i  in [1..deg-1] do
        k := RandomListMT(rs, [i..deg]);
        if k <> i then
            tmp := rnd[i];
            rnd[i] := rnd[k];
            rnd[k] := tmp;
            sgn := -sgn;
        fi;
    od;
    if even and sgn = -1 then
        tmp := rnd[deg];
        rnd[deg] := rnd[deg-1];
        rnd[deg-1] := tmp;
    fi;
    return rnd;
end;

Should we maybe document RandomListMT or some similar function and use this in similar situations? (There is also RandomIntegerMT(state, nrbits).)

[stevelinton]

Nice that it speeds it up. We should do this more widely for many reasons. The problem with skipping method selection entirely is that it would mean the function only worked for Mersenne Twister random sources and not other kinds (I presume). Do we care?

One possible solution would be have an Operation for IsRandomSource that returned a function that did RandomList or RandomInteger from the source.

So something like

foo := GetRandomListFunc(rs);
for .....
     k := foo([i...deg])

this lifts the method selection outside the loop. It's an approach we could adopt in (some) other situations where method selection overhead is a concern.

[frankluebeck]

I have made a few experiments. I'm a bit puzzled why the variants

...
    if IsMersenneTwister(rs) then
      foo := i-> RandomListMT(rs!.state, [i..deg]);
    else
      foo := i-> Random(rs, [i..deg]);
    fi;
    for i  in [1..deg-1] do
        k := foo(i);
...

and

...
    met := METHOD_2ARGS(Random, TypeObj(rs), TypeObj([1..deg])); 
    for i  in [1..deg-1] do
        k := met(rs, [i..deg]);
...

give no speedup at all compared to the version of FLOYDS_ALGORITHM from this proposal (although the method selection in the inner loop is skipped).

Independent of this (and more generally not only for Random) it would be nice if there was a fast function Method(oper, args...) that returns the first applicable method for oper with the given arguments (similar to what DoOperation...Args does in the kernel, but returning the method instead of calling it). This would use the method cache and often be better than METHOD_2ARGS. And this would provide a much more generic functionality than the proposed GetRandomListFunc.

[stevelinton]

@frankluebeck I can see good uses for method but also a lot of pitfalls -- one would need to be careful of things like TryNextMethod and RedispatchOnCondition. Also you run the risk of using one method throughout a loop when a better one became applicable because the type of an object changed during one of the early iterations. Method selection (except for the case of lists with mutable entries) takes only a fraction of a microsecond if the cache hits, which it almost always does, and I'd worry about code becoming more complex, less flexible and less robust for very small gains.

[stevelinton]

This has gone quiet. The discussion about Method or something similar could perhaps be moved to an issue on the mailing list. The only obstacle to merging this for now, I think, is the breakage in pperm.tst and maybe trans.tst. I could fix those by just regenerating the output in the test files, but it would be much better for the future to amend these tests and some of the functions they use to use their own random sources, so that they are not dependent on how often the global random source gets used elsewhere. @james-d-mitchell -- would you or one of your team be willing to do this.

[fingolfin]

Or instead don't let those tests output any values that depend on randomness!

In any case, I agree that this should just be merged. Sure, we can perhaps do "even better", but that should not be a reason to block an improvement.

[james-d-mitchell]

@stevelinton I can certainly fix the tests in pperm.tst and trans.tst, their output should not depend on randomness, I agree this is bad. I'll try to do it today, or tomorrow.

[stevelinton]

What I've done to test the random permutation generation is to use a new random source in the test, with a set seed, so it does depend on randomness in a sense, but isn't vulnerable to unexpected changes elsewhere. I agree though, that not using randomness unless it's actually needed is probably better.

[markuspf]

@james-d-mitchell did you have any chance to do this yet?

[james-d-mitchell]

@markuspf not yet, I don't have time at present. Maybe next week.

[fingolfin]

What's the status of this? At the very least it should rebased; might also wait for #810 to be merged and then make use of it to simpliify this PR a bit. And What is the status of those tests? @james-d-mitchell ?

Also, it seems @stevelinton is currently absent from discussions here, so somebody else may have to do the rebase etc. in a new PR?

[james-d-mitchell]

I see I'm mentioned here, but I can't locate this comment on github. Should I do something? I don't recall being involved in this...

…

On Fri, 13 Jan 2017 at 19:21 Max Horn ***@***.***> wrote: What's the status of this? At the very least it should rebased; might also wait for #810 <#810> to be merged and then make use of it to simpliify this PR a bit. And What is the status of those tests? @james-d-mitchell </~https://github.com/james-d-mitchell> ? Also, it seems @stevelinton </~https://github.com/stevelinton> is currently absent from discussions here, so somebody else may have to do the rebase etc. in a new PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#573 (comment)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/AGYFd1kksuVtVpCggGPITVMDoiA9VzOuks5rR85pgaJpZM4HRYw9> .[image: Web Bug from /~https://github.com/notifications/beacon/AGYFdwBkaGKrGHqTnpQXizj4HAYj8ccAks5rR85pgaJpZM4HRYw9.gif] {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/gap-system/gap","title":"gap-system/gap","subtitle":"GitHub repository","main_image_url":" https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png ","avatar_image_url":" https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in ***@***.*** in #573: What's the status of this? At the very least it should rebased; might also wait for #810 to be merged and then make use of it to simpliify this PR a bit. And What is the status of those tests? @james-d-mitchell ?\r\n\r\nAlso, it seems @stevelinton is currently absent from discussions here, so somebody else may have to do the rebase etc. in a new PR?"}],"action":{"name":"View Pull Request","url":" #573 (comment)"}}}

[markuspf]

@fingolfin I can do rebase and the like.

@james-d-mitchell The tests in pperm.tst used to be broken in this PR, let me try to rebase this PR and look whether this is still the case.

[james-d-mitchell]

Ok, I see get it, I'll try to fix it this week.

[stevelinton]

Rebased. There are still a couple of errors in pperm.tst

[fingolfin]

@stevelinton I fixed the pperm test, if you rebase this once more, we should hopefully see the tests passing

[james-d-mitchell]

@fingolfin thanks for fixing the tests and sorry I didn't find time to do it myself

[fingolfin]

@james-d-mitchell no worries :)

@stevelinton @ChrisJefferson Since PR #810 has been merged now, I suppose this PR should also be adapted to use InstallMethodWithRandomSource - doing that is really quite easy, too.

[fingolfin]

@stevelinton Any chance for a rebase?

@markuspf alternativly, perhaps you could revive PR #1080 ? This is now over a year old and still not merged.

[fingolfin]

This has been superseded by PR #1165