Mark FittingSubgroup and FrattiniSubgroup as nilpotent

[fingolfin]

The Frattini and Fitting subgroups of finite groups are always nilpotent.
See issue #398.

Please do not yet merge, I am still checking for some other potential changes along the same vein.

[fingolfin]

So there are more opportunities like this. E.g. various NormalSubgroups methods do not set IsNormalInParent.

OTOH, I also found code like this in the library:

    Assert( 1, IsAbelian( G ) );
    SetIsAbelian( G, true );

That strikes me as a good idea: We compute properties based on some theorems; but of course there can be bugs in the code, so if the assertion level is high enough, we verify that the theorem's really apply. So perhaps I should adapt the changes in this PR to do that, too? Thoughts?

[hungaborhorvath]

Indeed, this looks like a good idea. Probably setting properties/attributes should have such checks before them, but it might be very useful in other situations, as well.

[hulpke]

On Dec 14, 2015, at 11:23 AM, Max Horn notifications@github.com wrote:

So there are more opportunities like this. E.g. various NormalSubgroups methods do not set IsNormalInParent.

Careful: The stored `Parent’ might not be the group in which you compute normal subgroups.

[hungaborhorvath]

May I suggest also to change grp.gi:960 to InstallImmediateMethod instead of the current InstallMethod? That is, for nilpotent groups, GAP should know immediately that FittingSubgroup is itself.

[hulpke]

@hungaborhorvath
Unless you have a method that relies on knowledge of the Fitting subgroup (i.e. a method is installed for HasFittingSubgroup), making such a method immediate would not make any difference (apart from a call to FittingSubghroup for nilpotent groups being minusculy faster, but a similar amount of time having been spent before when the group was recognized to be nilpotent.
The purpose of the type system after all is not to prove theorems, but simply to allow for an effective selection of methods.

[hungaborhorvath]

@hulpke I agree that currently there are not many methods using HasFittingSubgroup, though, CRISP's AbelianMinimalNormalSubgroups is there. Nevertheless, I disagree about not setting such attribute. On the one hand, I think this is a perfect situation of not spending time on setting an attribute with ImmediateMethod. I would have thought this is why ImmediateMethod was created. I might be wrong, though. On the other hand, the fact that currently there are not many (or none) methods using HasFittingSubgroup does not mean that later on there will be none, and missing this ImmediateMethod then can bite developers. (Of course it could be set then, but I always lived by "Tomorrow never comes" )

[hungaborhorvath]

Actually, currently the InstallMethod for FittingSubgroup with filters [ IsGroup and IsNilpotentGroup ] does not work, because [ IsGroup and IsFinite ] ranks higher for some reason. In particular, for big nilpotent groups it takes a long time, partly because it will need to factor the size. Not to mention the fact that afterwards even a G=FittingSubgroup(G); check will take time, as well.

Thus, even if the method will not be turned Immediate, the ranks should still need to be adjusted.

[fingolfin]

I increased the rank of the IsGroup and IsNilpotentGroup method (we can make it immediate some other time), and added some tests.

Does it look sensible now?

[hungaborhorvath]

I am not sure how to comment on codelines, but I managed to somehow copy this here:

+gap> f := FrattiniSubgroup(g);
+<group of 2x2 matrices of size 2 over GF(5)>
+gap> HasIsNilpotentGroup(f);
+true+gap> f := FrattiniSubgroup(g);
+<group of 2x2 matrices of size 2 over GF(5)>
+gap> HasIsNilpotentGroup(f);
+true
+gap> p := SylowSubgroup(g, 2);;
+gap> HasIsNilpotentGroup(p);
+true
+gap> HasIsNilpotentGroup(FrattiniSubgroup(p));
+true
+gap> g := SL(IsPermGroup,2,5);;
+gap> f := FrattiniSubgroup(g);;
+gap> HasIsNilpotentGroup(f);
+true
+gap> p := SylowSubgroup(g, 2);;
+gap> HasIsNilpotentGroup(p);
+true
+gap> HasIsNilpotentGroup(FrattiniSubgroup(p));
+true
+gap> p := SylowSubgroup(g, 2);;
+gap> HasIsNilpotentGroup(p);
+true
+gap> HasIsNilpotentGroup(FrattiniSubgroup(p));
+true
+gap> g := SL(IsPermGroup,2,5);;
+gap> f := FrattiniSubgroup(g);;
+gap> HasIsNilpotentGroup(f);
+true
+gap> p := SylowSubgroup(g, 2);;
+gap> HasIsNilpotentGroup(p);
+true
+gap> HasIsNilpotentGroup(FrattiniSubgroup(p));
+true

I do not understand why these FrattiniSubgroup calls are in the FittingSubgroup file and not in the FrattiniSubgroup file....

[hungaborhorvath]

Otherwise looks fine to me, although I would really appreciate an immediatemethod. :-)

Did you decide not to put in the Assertion tests?

[fingolfin]

For commenting on lines in the diff, please [read this page)[https://help.github.com/articles/commenting-on-the-diff-of-a-pull-request/).

I fixed the tests (thanks, not sure how that happened), and while at it, extended them some more. (For now, this is a fairly random set of tests. Once @ChrisJefferson's code gets a bit easier to use, we can try to improve the quality in terms of coverage.

I also added the asserts -- I simply forgot about that idea. But now I shold run the full tests suite to make sure they don't cause horrible slowdowns or lockups.

As to making that one method immediate: Well, in principle I don't see a strong reason not to, but making something immediate can have weird consequence that are hard to anticipate, so for now I"d like to play it safe... We can revisit that in the future, though (e.g. right after this gets merged, you are welcome to open a new PR which changes the method to immediate).

[hungaborhorvath]

Did you want to have FittingSubgroup here?

[fingolfin]

@hulpke Any thoughts or concerns on this pull requests from your side?

Otherwise, I think I'll just merge it myself in a few days... ;-)

[hulpke]

@fingolfin All fine with me. Thanks for asking!