WIP: Add test for NiceMonomorphismByDomain

[fingolfin]

This grew out of my attempt to fix issue #27 (the final fix is now PR #41).

As a first step, I added a test to exercise NiceMonomorphismByDomain, the only place calling the old orb hashtable API.

Unfortunately, the new test fails 😂 as in: NiceMonomorphismByDomain is maybe incorrect. I guess this underlines how important it is to have test cases for all code, even the "obviously" correct code.

Note that the test fails if one forces the parent of the group in question to compute its nice monomorphism first, i.e.: insert NiceMonomorphism(Parent(g)); before the Size(g) to ensure that.

So my idea of "simply" replacing the old by the new hashtable API is a no-go because I can't really test the change... gotta figure out what's wrong with this code first. Actually after understanding what's going on, I decided the test is "good enough" to verify the hashtable related changes I made work right; I put them into PR #41)

CC @jdebeule

[codecov]

Codecov Report

Merging #38 (c84280e) into master (115b07d) will increase coverage by 0.89%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head c84280e differs from pull request most recent head a287456. Consider uploading reports for the commit a287456 to get more accurate results

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   47.62%   48.51%   +0.89%     
==========================================
  Files          34       34              
  Lines       17927    17913      -14     
==========================================
+ Hits         8537     8690     +153     
+ Misses       9390     9223     -167     

Files Changed	Coverage Δ
tst/testall.g	89.65% <100.00%> (+0.36%)	⬆️

... and 7 files with indirect coverage changes

[fingolfin]

I dug a bit and this is the problem: the test elm in g above is implemented by this method:

InstallMethod( \in,
    "by nice monomorphism",
    IsElmsColls,
    [ IsMultiplicativeElementWithInverse,
      IsGroup and IsHandledByNiceMonomorphism ],
    0,

function( elm, G )
    local   nice,  img;
 
    nice := NiceMonomorphism( G );
    img  := ImagesRepresentative( nice, elm:actioncanfail:=true );
    return img<>fail and img in NiceObject( G )
       and PreImagesRepresentative( nice, img ) = elm;
end );

Now everything works fine until the final check PreImagesRepresentative( nice, img ) = elm;. Here we observe this:

gap> elm2:=PreImagesRepresentative( nice, img );
< a collineation: <cmat 2x2 over GF(5,4)>, F^125>
gap> elm=elm2;
false
gap> NamesOfComponents(elm);
[ "mat", "fld", "frob" ]
gap> elm!.mat = elm2!.mat;
true
gap> elm!.fld = elm2!.fld;
true
gap> elm!.frob = elm2!.frob;
false
gap> elm!.frob ; elm2!.frob;
FrobeniusAutomorphism( GF(5^4) )
FrobeniusAutomorphism( GF(5^4) )^3

So, yeah: those preimages are not equal. The differ on pg -- but they are equal when restricted to the subspace sub, which is only defined over GF(5) after all. So those two induce a monomorphism there after all.

So CollineationGroup(sub); returns a group generated by collineations of the parent geometry. But two such collineations of pg, when restricted to sub, might induce identical maps (e.g. the identity on pg and the Frobenius automorphismonpgboth induce the identity onsub`).

Based on this, I think the real problem is that the Frobenius of GF(5^4) is used as a generator for the subgeometry. I'll see if I can fix that

[fingolfin]

There is some discussion mentioning CollineationGroup and issues with nice monomorphisms in the package manual, but I don't understand what it says... sigh

[fingolfin]

Upon re-reading a few times, I think I get it now: applied to our example, where the subgeometry sub of $PG(1,GF(5^4))$ is isomorphic to $PG(1,GF(5))$, it says that the collineation group of sub is defined to be a certain conjugate of the collineation group of $PG(1,GF(5))$?

I think this is meant to deal with subgeometries isomorphic to $PG(1,GF(5))$ but different from the "canonical" (with respect to the standard frame) copy of $PG(1,GF(5))$. But in the example here, this is already the group in question.

So more is going on: indeed, the collineation group here is $PGL(2,5)$, isn't it, of size 120. But the collineation group the code produces has size 480. Indeed, what we actually get is the collineation group we are looking for but extended by the Frobenius of the large field $GF(5^4)$, which has order 4.

If we just omit that generator, we actually get the "right" group. But that won't work for e.g. $PG(1,GF(5^2))$, because there we need a Frobenius of $GF(5^2)$; but that just isn't an element of the "big" group $P\Gamma(2,GF(5^4))$ -- indeed I don't think $P\Gamma(1,GF(5^2))$ is a subgroup of $P\Gamma(2,GF(5^4))$ (at least not in a "natural" way). That's because the Frobenius $f_{5^2}$ of $GF(5^2)$ has two "lifts" to $GF(5^4)$, both of order 4, namely $f_{5^4}$ and $f_{5^2}^3=f_{5^2}^{-1}$... the group generated by $f_{5^2}$ is a quotient of $f_{5^4}$ but the quotient map has no section...

OK, so maybe this is intentional, and the collineation group of the subgeometry is meant to be given "extended by the Frobenius of the parent geometry" ? But that really should be in the documentation then? (Maybe it is, but then I didn't find it, and would appreciate pointers as to where I can read more).

If the intent is to use this "extended" collineation group, then a possible fix for the issue here would be to tweak NiceMonomorphismByDomain to extend the group action to also consider the full "Frobenius" part separately (what I mean is this: right now we use the action on AsList(Points(geom)). We could add to this the orbit of a primitive element of the large field under the action of just the Frobenius part of each collineation (i.e. collineation!.frob).

Alternatively, a bigger change would be to drop the illusion that the collineation group of the subgeometry is a subgroup of the collineation group of the parent geometry. This is wrong in this example here and also wrong in general, so I am not sure why the code seemingly attempts to pretend otherwise?