Fix implication for IsSubsetLocallyFiniteGroup; and teach Units(GF(q)) its size

[fingolfin]

IsFFECollection and IsMagma does not imply IsSubsetLocallyFiniteGroup, as e.g. any finite field GF(q) satisfies the former, but clearly not the latter

[fingolfin]

@ThomasBreuer suggested that instead of removing this implication, one could replace it by the correct implication

InstallTrueMethod( IsSubsetLocallyFiniteGroup, IsFFECollection and IsMagmaWithInverses );

UPDATE: fixed to use IsMagmaWithInverses as suggested by Thomas (instead of the accidentally copy&pasted IsMagma)

[fingolfin]

@hulpke suggested that one should check why this implication was added in the first place. I did research this before making this PR, but did not add this in, as the result turned out to be not very helpful. It was added in this commit (not available in the public history):

commit 570c33826714a680e7c6d1a14b6d1dc6590978f9
Author: Alexander Hulpke <hulpke@math.colostate.edu>
Date:   1997-10-09 17:59:22 +0000

    Introduced property 'IsSubsetLocallyFiniteGroup' that is implied by
    'IsPermCollection' or 'IsFFECollection'. Removed the deductions that
    such groups are automatically finite, rerouted this via
    'IsFinitelyGeneratedGroup'.
    At the moment this should not create any problems but avoids trouble if we
    ever get infinite permutation groups. AH

 lib/ctblfuns.gi | 25 ++++++++++++++++---------
 lib/field.gd    |  4 +---
 lib/grp.gd      | 21 +++++++++++++++++++++
 lib/grp.gi      | 27 ++++++++++++++++++---------
 lib/grpperm.gd  |  6 +++---
 5 files changed, 59 insertions(+), 24 deletions(-)

Unfortunately, the commit message does not really explain why IsSubsetLocallyFiniteGroup is needed. So I next went and checked how IsSubsetLocallyFiniteGroup is used today. It turned out that no distributed or other package that I know uses it (it does occur in the utils package, but only as part of tests resp. documentation which prints known properties of some objects). This leaves the library. There, it turns out, no code directly checks IsSubsetLocallyFiniteGroup or uses it as a filter. There is one check in lib/grppclat.gi:211 but it is commented out (and was already added to the repository in commented out form).

So, the only use of this property is via its implications, and the only relevant one (as far as I could tell) is this:

# this true method will enforce that many groups are finite, which is needed
# implicitly
InstallTrueMethod( IsFinite, IsFinitelyGeneratedGroup and IsGroup
                             and IsSubsetLocallyFiniteGroup );

So, it ensures that finitely generated subgroups of permutation groups are marked as finite, which is great.

The implication this PR removes presumable was meant to ensure that finitely generated subgroups of the unit group of a finite field would automatically be marked as finite. This is now gone. So the effect is that e.g. HasIsFinite(Group(Z(2^5))); returns true on master and false on this PR.

So, I guess this is useful, and we can follow @ThomasBreuer's suggestion to modify the true method instead (although I wonder if it is really that useful to justify a "true method"? I remember @hulpke being very concerned about their performance impact ;-). If we go that route, I'd also add some tests for the implications, e.g. the HasIsFinite(Group(Z(2^5)));

While we are at it, perhaps then one should also replace the current implication

InstallTrueMethod( IsFinite, IsFFEMatrixGroup and IsFinitelyGeneratedGroup );

by this more general one?

InstallTrueMethod( IsSubsetLocallyFiniteGroup, IsFFEMatrixGroup );

[hulpke]

@fingolfin (Answer will come in chunks to the several topics):

I have no concern about InstallTrueMethod, as this is just changing bit masks for certain properties. When running through code this imposes effectively zero cost, even when called often.
Also implications from implications are sorted out once at the start and don't recur again. (I have no opinion about the name

My concern (past and now) is about Immediate methods which involve an explicit function call. They have epsilon cost but when called often (they might need to be called if the type of an object changes) that can sum up. Also the result of one immediate method can trigger other immediate methods. (In around ~1998 we observed such effects in concrete code.)

[hulpke]

@fingolfin Thanks for digging out the commit message (which I was unable to find in git). Let me describe what I recall these filters and the TrueMethod implications (indeed, that's all what they are used for) were supposed to achieve:

Many of the library routines (and in particular basic fallback routines) require finiteness, and thus it seems to be right to have them require IsFinite. Also, IsFinite has a surprisingly high filter value, and if forgotten to require may rank a good specialized method lower, than a slow, generic one that only requires IsFinite.

A solution to both problems seemed to be to have the category of certain objects to imply (through true methods) finiteness.

In some cases (e.g. PcGroups) this is easy and clear. In other cases (say permutation groups, if we assume IsPerm has a finite support) it could fail to hold if we ever want to introduce "limit" objects such as the finitary symmetric group. A cheap way around this issue (which will imply finiteness through a combination of filters) is SubsetLocallyFinite together with FinitelyGenerated (which can be set for no cost by ...ByGenerators.

I'm not insisting on this particular solution, but if we remove it we should be careful that the IsFinite implication does not go away.

[hulpke]

With the changed implication I am happy with the PR.

[fingolfin]

Thanks @ThomasBreuer and @hulpke for taking the time to review this and lots of helpful remarks!