Make FittingSubgroup method immediate for nilpotent groups

[hungaborhorvath]

This is a continuation of #400 to make FittingSubgroup method immediate for nilpotent groups.
Further, FrattiniSubgroup is rewritten to FittingSubgroup in the tst file,
and the immediatemethod is checked, as well.

[hulpke]

I'm still not persuaded of the benefit of making something like this an immediate method. The only benefit will be that a nilpotent group immediately has a fitting subgroup (i.e. HasFittingSubgroup is set). This is a filter which is nowhere used in the library.
The price is that for any group that happens to know it is nilpotent (e.g. any cyclic or trivial group) this immediate method runs and causes a type change that takes a small amount of time. Is that really worth it?

[hungaborhorvath]

@hulpke You are right, currently there is no method using HasFittingSubgroup. This is not without precedent, though. CRISP 1.3.9 had HasFittingSubgroup in the method selection for SolvableSocleComponents and for AbelianMinimalNormalSubgroups. They are gone in the most recent version.

[fingolfin]

There is not just a speed penalty, but also a memory penalty: Storing the attribute takes up memory, e.g.:

gap> G:=CyclicGroup(IsPermGroup,10);
Group([ (1,2,3,4,5,6,7,8,9,10) ])
gap> MemoryUsage(G);
230
gap> HasFittingSubgroup(G);
false
gap> FittingSubgroup(G);
Group([ (1,2,3,4,5,6,7,8,9,10) ])
gap> MemoryUsage(G);
246

Granted, it's not much, but only because we already start out with quite an overhead

[hungaborhorvath]

Abelian groups do not know their centers immediately, either. I personally think that this behaviour should also be changed, but if people are against it, then I will just leave everything as it is.

[fingolfin]

The problem is that all these "immediate" methods can add up. When you need to do millions of computations, the added time and memory requirements can indeed become outright punishing.

This is why I am not so happy about the idea of ading these, unless you have explicit justifications for this, like "this computation that came up in my research works a lot, lot faster thanks to this tweak".

This is a particular instance of a general problem we have (and that also comes up in your issues #585 and #594): If we are clever, we can sometimes solve problems that are otherwise much harder or impossible to solve. But if we spend too much time trying to be clever, we may make a previously easy problem slow. And ad-hoc hacks that work great in one example can have horrible results in others. The matrix group code which works by nice monomorphisms is another example: @hulpke put an optimization in there which tries to find smaller degree perm reps the first time a nice monomorphism is computed. In some examples, this has tremendous advantages. In others, you can end up spending much time (minutes, for me) findinig this better presentation, when the direct computation would have finished in seconds without this optimization.

Hence, I believe that to enable serious"low-level computations, it is important to either be able to deactivate these "optimizations" (for matrix group example, @hulpke added a "cheap" option for this reason, which is very helpful), or to allow access to equivalent functionality that bypasses the tricks; or else be very, very, VERY sure your optimization is never worse than the "unoptimized" code.

In this particular example, I wonder if we could add a kind of "delayed immedate method" -- let me illustrate what I mean with the following description (which wouldn't work in the current setup, of course, I merely want to illustrate a point):
Instead of storing the center of an abelian group immediatly, we could only set the HasCenter filter (a single bit, the time and storage overhead for this should be tiny). Then, method selection can benefit from it immediately. But only if somebody explicitl asks for the center would we actually set the attribute.

However, I am still not sure whether it would be worth it... And the whole filter based system only works for positive implications (i.e. you can install code for groups known to be solvable -- but not code for groups known to be non-solvable), and it cannot take interaction between multiple parameters into account very well; and in the end, the whole filter rank system is simple and elegant, but inadequate to deal with all situations. To find out which method for a given operation really is best, you need to take all parameters into account at the same time, and possible solve an optimization problem.

[hungaborhorvath]

@fingolfin I like the idea of having a delayed immediate method, I do not see any disadvantage using such a system (apart from needing someone to set it up) for attributes. BTW, why does the method selection work in such a way that only positive properties are checked? I guess those are used most of the time, but is it really that problematic to set up both ways? Or is there some big efficiency problem? I have no idea about such core coding, so I genuinely have no idea.

I agree that sometimes looking for a clever algorithm may take more time than to actually doing it in some (possibly brute force) way. I acknowledge this, but I am not wired like this. I personally would rather think hours/days/weeks on a problem trying to find a tricky solution than spend 30 minutes actually computing it. I guess this annoys people, who are more careful. Anyway, I will try to be more careful in the future. BTW I always thought about these PRs as possibilities to ask people whether a change would be useful or not. If not, then we can always simply reject it.

As for the current issue: so far nobody seems to want this. If there will be nobody else coming up with anything new in a few days, I will simply close this PR. In that case though, the tst typo should be corrected (currently FittingSubgroup.tst calls FratiniSubgroup at some point, and FrattiniSubgroup.tst makes exactly the same call).

[bh11]

In a sense, we already have this kind of "delayed immediate methods", all the filters CanEasilyComputeXXX are of that kind. Still, I doubt that this is useful in trivial cases (like an abelian group knowing its centre) – if a method profits from knowing the centre, it probably expects the group to be non-ablelian, and one should really add a specialised method for abelian groups if available.

Btw. also the only (past) example "in the wild" using HasFittingSubgroup for method selection wouldn't have improved the situation for nilpotent groups: in the nilpotent case, it would have performed the same steps as the generic method without known Fitting subgroup.

So I'd suggest to wait until someone actually needs such immediate or semi-immediate methods.

[hungaborhorvath]

Well, all right then. I believe that the author of the only past usage of HasFittingSubgroup has settled the matter. :-) I am closing this PR, and will open a trivial one for the typo change.