Fix LatticeSubgroup sometimes returning wrong results for matrix groups

[fingolfin]

... if used on a group which is handled by a NiceMonomorphism, which normally includes all matrix groups. Also fix an IsFinite method for non-rational cyclotomic matrix groups.

In both cases, the code wrongly assumed that the Image(NiceMonomorphism(g)) equals Image(NiceMonomorphism(g),g). The solution is to use NiceObject(g) which is also cached, as it is an attribute.

Also change a few more places to use NiceObject(g); while they were correct, using NiceObject is potentially more efficient.

Fixes #4717

[laurentbartholdi]

I wrote a comment in another thread, that it seems dangerous to me to have both NiceObject and NiceMonomorphism in code, with the secret guarantee that the range of the latter is IdenticalObj to the parent of the former.

I see that the code contains quite a few RestrictedNiceMonomorphism, which precisely construct the restricted map I was thinking about. For these, it is safe to use Image(mono), which should be IdenticalObj to NiceObject(g), and cached.

How about the following cleanish way of implementing the guarantee above? each NiceObject has an attribute ComingFrom which is the NiceMonomorphism used to produce it. So for example HallSubgroup would become

function(g,l)
local mon,h;
   niceg:=NiceObject(g); 
   h:=HallSubgroup(niceg,l);
   if h = fail then
       return fail;
   elif IsList(h) then
       return List(h, k -> PreImage(ComingFrom(niceg), k));
   elif IsGroup(h) then
       return PreImage(ComingFrom(niceg),h);
   else
       Error("Unexpected return value from HallSubgroup");
   fi;
end);

where I replaced all the mon by ComingFrom(niceg).

[fingolfin]

I wrote a comment in another thread, that it seems dangerous to me to have both NiceObject and NiceMonomorphism in code, with the secret guarantee that the range of the latter is IdenticalObj to the parent of the former.

There is no "secret guarantee", and the the latter need not be "the parent of the former". The guarantee is that that NiceObject(G) = Image(NiceMonomorphism(G),G), and this is not "secret", it is documented explicitly.

I see that the code contains quite a few RestrictedNiceMonomorphism, which precisely construct the restricted map I was thinking about:

... and they do so with precisely the overhead I was thinking about :-), namely: the resulting map can be less efficient, be it because it is a new wrapper object around the actual homomorphism or be it because it was rewritten as a GHBI (group homomorphism by images), or for some other reasons.

For these, it is safe to use Image(mono), which should be IdenticalObj to NiceObject(g), and cached.

It is still not true that this image need be identical, only that it is equal. And one of the various motivations for NiceObject(g) is precisely to have it cached.

How about the following cleanish way of implementing the guarantee above? each NiceObject has an attribute ComingFrom which is the NiceMonomorphism used to produce it.

I don't see how this is any "cleaner" than what have, or for that matter, "safer": one could apply the same concerns about this: "is it guaranteed that ComingFrom(NiceMonomorphism(G) really is the same as G" etc.

But it certainly would be different, and using it would require adapting the whole library and lots of packages.

So for example HallSubgroup would become

function(g,l)
local mon,h;
   niceg:=NiceObject(g); 
   h:=HallSubgroup(niceg,l);
   if h = fail then
       return fail;
   elif IsList(h) then
       return List(h, k -> PreImage(ComingFrom(niceg), k));
   elif IsGroup(h) then
       return PreImage(ComingFrom(niceg),h);
   else
       Error("Unexpected return value from HallSubgroup");
   fi;
end);

where I replaced all the mon by ComingFrom(niceg).

[laurentbartholdi]

Why not IsFinite(NiceObject(G)) ?

[fingolfin]

As explained in the comment right above this line, there is no method to compute NiceObject in this case. We could also change the existing NiceObject to apply to arbitrary groups, not just those in the filter IsHandledByNiceMonomorphism, but that's a bigger change with potentially further implications, and so I decided to go this way, as it minimizes the risk of a regression. One could investigate the mentioned alternative in a separate PR, but I'd rather not mix it with a bugfix.

[laurentbartholdi]

Ah, uh, sorry, I didn't read the comments.

…

On Mon, Dec 13, 2021 at 2:49 PM Max Horn ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/grpramat.gi <#4718 (comment)>: > @@ -251,7 +251,9 @@ function( G ) # if not rational, use the nice monomorphism into a rational matrix group if not IsRationalMatrixGroup( G ) then - return IsFinite( Image( NiceMonomorphism( G ) ) ); + # the following does not use NiceObject(G) as the only method for + # that currently requires IsHandledByNiceMonomorphism + return IsFinite( Image( NiceMonomorphism( G ), G ) ); As explained in the comment right above this line, there is no method to compute NiceObject in this case. We could also change the existing NiceObject to apply to arbitrary groups, not just those in the filter IsHandledByNiceMonomorphism, but that's a bigger change with potentially further implications, and so I decided to go this way, as it minimizes the risk of a regression. One could investigate the mentioned alternative in a separate PR, but I'd rather not mix it with a bugfix. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#4718 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARAQUGW2A4NOFIRL3R3SRLUQX2VXANCNFSM5J3AUWEQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Laurent Bartholdi laurent.bartholdi<at>gmail<dot>com Fachrichtung Mathematik, Universität des Saarlandes Postfach 151150, 66041 Saarbrücken, Germany Tel. +49 681 3023227, Sekr. +49 681 3023430

[hulpke]

The only problematic issue is in Image(hom), which predates the observation that it can be most effective to inherit nice monomorphisms. But for consistency using NiceObject in place of Image(hom,G) is probably best.