kernel: simplify bits of the transformations and partial perms code

[fingolfin]

No description provided.

[fingolfin]

I wanted to extend this a bit more, but while doing so uncovered a bug in ProdPPerm4Perm2 (?), as this shows (I'll try to debug it tomorrow)

gap> f := PartialPerm(Concatenation(List([1 .. 65], x -> 0), [1, 70]));
[66,1][67,70]
gap> f * (1,65536);
[66,65536][67,70]
gap> g := PPerm4(Concatenation(List([1 .. 65], x -> 0), [1, 70]));
[66,1][67,70]
gap> g * (1,65536);
[66,1][67,70]
gap> f = g;
true

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@4402d59). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3219   +/-   ##
=========================================
  Coverage          ?   84.84%           
=========================================
  Files             ?      686           
  Lines             ?   330604           
  Branches          ?        0           
=========================================
  Hits              ?   280486           
  Misses            ?    50118           
  Partials          ?        0

[coveralls]

Coverage increased (+0.003%) to 84.645% when pulling 7bdc71a on fingolfin:mh/trans-pperm into 4402d59 on gap-system:master.

[wilfwilson]

gap> g := PPerm4(Concatenation(List([1 .. 65], x -> 0), [1, 70]));

What do you mean by writing the command PPerm4? That's not something you can call from GAP, is it?

[fingolfin]

@wilfwilson that command comes from pperm.tst. Anyway, I already located the bug: the result of DEG_PERM2 is an UInt, and may not fit in an UInt2 -- yet several functions in pperm.cc store it in one. As a result, a permutation of degree 65536 is treated as if it had degree 0... I am working on a fix and new tests, which I'll submit in a separate PR soonish.

[fingolfin]

Here is a simple example that showcases an instance of the bug:

gap> f:=PartialPerm([0,0,1,5]);
[3,1][4,5]
gap> (3,65536) * f;
[3,1][4,5]

[fingolfin]

Fix is in PR #3220 -- let's not merge this PR here (3219) until #3220 is merged; then I'll rebase it with some other minor tweaks.

[wilfwilson]

If it's possible to re-request a review once you've done that, then please do, otherwise please write a comment in this thread so that I am alerted :)

[james-d-mitchell]

This looks all good to me, can you enlighten me as to why you're adding static here?

[fingolfin]

My longterm plan is to make all functions static that are not explicitly for external use, so as to avoid the situation were people "accidentally" use them w/o being aware of them being private. See also PR #3204. That doesn't mean they have to stay static/private, but if somebody needs to use a function, we (well, me, at least) would prefer to know about it explicitly. That frees us from the burden of having to verify for every single kernel refactoring whether maybe some package is using that interface anyway (though, luckily, we now again compile and at least test-load almost every distributed package again, including digraphs and semigroups, so this is less of a problem than it used to be).

The other advantage is that it may help the compiler with some optimizations and allows for a slightly slimmer binary (but these are relatively minor points).

BTW, if you have any need to invoke any of these function directly from, say, semigroups (instead of going via e.g. QUO in this case), please tell us, then of course we can leave of the static from those, and/or find some other means to accommodate you; the goal here really isn't to annoy anybody, but rather to prevent future annoyances.

[fingolfin]

Rebased now; also replaced even more "quotient of trans or pperm by perm" code

[fingolfin]

I just learned that INV for T_PERM2/4 is actually guaranteed to return something in the same rep, so I guess we could optimize this code again by calling ProdPPerm2/4Perm2/4 directly, instead of PROD. But will save just a few cycles to load the function pointer from memory (memory that likely is in the cache anyway), so I am not sure it's worth it? (The extract functions of course then also would in turn require more cache, so I don't dare predict whether it would have positive, negative or no effect at all...)

So unless somebody is really concerned about this, I'd leave it at this single generic function for now...

As a matter of fact, I just noticed that we can even get rid of this completely, in which case QuoDefault will be used, which does the right thing anyway.