Add method for Socle for finite nilpotent groups #402

hungaborhorvath · 2015-12-14T17:42:20Z

For finite nilpotent groups the Socle is simply the direct product of the Omega groups of the center of Sylow subgroups. See #385.

Maybe an IsNilpotentGroup check could be forced in the other Socle methods with a redispatch?

fingolfin · 2015-12-14T18:03:56Z

lib/grp.gi

+        SetCentralizerInParent(prodH, Parent(prodH));
+        SetIsNormalInParent(prodH, true);
+      fi;
+    od;


You created prodH as a subgroup of G here, so I am pretty sure (but have not verified) that G is always the parent of prodH here -- so you could simplify the code a lot.

Moreover, I don't think it's worth it setting this information for the intermediate groups prodH which you are throwing away in the next loop operation anyway. It should be enough to set this information for the final value of prodH only, i.e. outside the loop. Or am I missing something?

I am not entirely sure, but I think in one of my examples prodH did not have a parent...

However, in any case the parent might not be G, e.g. if G is a FittingGroup of some bigger group, then prodH would inherit the parent of G (see examples from #398).

I set these filters for the internal groups so that GAP would know about them. It might be a waste of memory, or the user might use these groups later on for something else. I can move it outside of the loop if that is preferred.

Right, I see your point regarding parents, I forgot that G could already have another parent. Drats.

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

In lib/grp.gi:

now socle is product of Omega of the center of its Sylow subgroups

for H in SylowSystem(G) do

p := PrimePGroup(H);

prodH := ClosureSubgroupNC(prodH, Omega(Center(H), p));

# prodH is central in G, set some properties and attributes accordingly

SetIsAbelian(prodH, true);

if not HasParent(prodH) then

SetParent(prodH, G);

SetCentralizerInParent(prodH, G);

SetIsNormalInParent(prodH, true);

elif IsSubgroup(G, Parent(prodH)) then

SetCentralizerInParent(prodH, Parent(prodH));

SetIsNormalInParent(prodH, true);

fi;

od;

You created prodH as a subgroup of G here, so I am pretty sure (but have not verified) that G is always the parent of prodH here

I would not want to bet on this. There even is no guarantee that Subgroup(G,l) always returns an object whose parent is set to be G (it might instead be a parent of G. Also code such asClosure’ might try to be clever and realize the closure is already a known group and then return this group (with a stored, different, parent).

When Parent’ was created it had been intended to serve a similar role as the group of the FamilyPcgs, but it turns out that with subgroups of subgroups it was not possible to formulate a consistent policy on howParent’ should be set.

Moreover, I don't think it's worth it setting this information for the intermediate groups prodH which you are throwing away in the next loop operation anyway. It should be enough to set this information for the final value of prodH only, i.e. outside the loop. Or am I missing something?

—
Reply to this email directly or view it on GitHub.

@hulpke You are of course right about parent (and I was utterly wrong). @hungaborhorvath already convinced me about that :)

hungaborhorvath · 2015-12-14T20:55:37Z

Added RedispatchOnCondition checking for finiteness and nilpotency.

hungaborhorvath · 2015-12-14T22:30:13Z

As far as I can tell, in the main library there is only one other method, which is for permutation groups, and that exits immediately if the group is not primitive. About packages I am unsure, CRISP certainly has a solvable Socle method and tests for it.

I certainly think that an IsFinite and IsNilpotent check should be the first to do, and I am happy to enforce it by some value if that is the consensus. I would like to ask for some help, though, because I have no experience in the ranking of methods, so I am not sure what numbers are big or small, or if this should be set maximum possible (I recall reading in one of the files something like that).

So, should the code enforce an IsNilpotentGroup check, and what should the rank value be?

hungaborhorvath · 2015-12-16T08:02:06Z

Sorry, but please, do not merge this!

I just realized that grppc.gi:2559 already has this method for p-groups. (The code did not have the same formatting as the others, so

grep "Method( Socle" lib/*

did not give it to me when I first looked for methods.) So this code maybe should go as a rewriting of grppc.gi:2559? (BTW, that code does not seem to be correct, because it does not require to group to be finite....)

Second, right now I am not sure which is more efficient: computing the center first and then compute its Sylows and their omega, or the other way around. There are other considerations, as well, e.g. @bh11 in #398 said that for particular group presentations center might take very long to compute.

In any case, this still needs working on from my part. TBC

fingolfin · 2015-12-16T11:29:46Z

In GAP, IsPGroup indicates a finite p-group according to the documentation. So that method in grppc.gi should be OK.

Grepping for tht string is a very unsafe way to find methods for an operation. Besides troubles with even slight changes in formatting, it ignores functions declared in grp/ etc., in the kernel, and in packages.

Better to ask GAP for a list of available methods. Indeed, I wrote a helper function that shows all available methods for a given operation / attribute / property. Perhaps we could include something like that with GAP? Here it is:

ShowAllMethods := function(oper)
    local arity, methods, num, i, idx, j;

    for arity in [0..6] do
        methods := METHODS_OPERATION( oper, arity );
        num := Length(methods)/(4+arity);
        if num = 0 then continue; fi;
        Print("Arity ", arity, "\n");
        for i in [1..num] do
            idx := (i-1) * (4+arity);
            Print(i, ": ", methods[idx + (arity+4)], "\n");
            Print("   rank ");
            Print_Value_SFF(methods[idx + (arity+3)]);
            Print("\n");
            for j in [1..arity] do
                Print("   ", Ordinal(j)," argument filters ",
                      NamesFilter(methods[idx + 1 + j]), "\n");
            od;
            if FILENAME_FUNC(methods[idx + (arity+2)]) <> fail then
                Print("   file ",
                      FILENAME_FUNC(methods[idx + (arity+2)]), ":",
                      STARTLINE_FUNC(methods[idx + (arity+2)]), "\n");
            fi;
        od;
        Print("\n");
    od;
end;

hungaborhorvath · 2015-12-16T12:39:15Z

Wow, this code is great. Would it be possible to put it into the main GAP? I think it would help developers a lot.

stevelinton · 2015-12-16T15:45:56Z

There is BrowseGapMethods() in the browse package.

hungaborhorvath · 2015-12-19T20:42:27Z

Ok, I reworked the method:

RedispathcOnCondition now uses rank RankFilter(IsFinite and IsSolvable)+1 so that this method should run before the solvable methods. The IsPermGroup method is still first, though.
Replaced Center(SylowSystem(G)) by SylowSystem(Center(G)), because for big permutation groups the second is much more efficient:

gap>k := 100;; P := SylowSubgroup(SymmetricGroup(4*k), 2);; A := Group((4*k+1, 4*k+2, 4*k+3));; G := ClosureGroup(P, A);; IsNilpotentGroup(G);;
gap> SylowSystem(Center(G)); time; 
[ <permutation group with 3 generators>, 
  <permutation group with 1 generators> ]
10664

vs

gap>k := 100;; P := SylowSubgroup(SymmetricGroup(4*k), 2);; A := Group((4*k+1, 4*k+2, 4*k+3));; G := ClosureGroup(P, A);; IsNilpotentGroup(G);;
gap> SylowSystem(G); time;
[ <permutation group with 397 generators>, 
  <permutation group with 1 generators> ]
26612
gap> Center(SylowSystem(G)[1]); time; 
<permutation group of size 8 with 3 generators>
71272
gap> Center(SylowSystem(G)[2]); time;
<permutation group with 1 generators>
0

For polycyclic groups I did not see any significant difference between the two methods.

Finally, I moved all attribute settings to outside of the loop so that only Socle(G) will obtain attributes and properties. I put in a check for CanComputeIsCheck so that we would not need to wait for a possibly long IsSubgroup check to happen for getting the result. Anyway, if the consensus is to get rid of these attribute settings, then I will do so.

@fingolfin The IsPGroup method is ranked lower, because RankFilter(IsPGroup) is only 41, while RankFilter(IsFinite and IsNilpotentGroup) is 52. Actually, this ranking goes against the manual, because checking RankFilter(IsFinite and IsPGroup) gives 54, so GAP does not think that a PGroup is necessarily finite, at least according to the ranking. Should I post an issue about this?

The PR is again ready for review. Thank you for all the help.

hungaborhorvath · 2015-12-20T13:58:07Z

@hulpke Decided to remove RedispatchOnCondition and force the checks in the code. Increased the rank of the method, as well, so that it is called with the same rank as if IsFinite and IsNilpotentGroup properties are already true. Is this the way you wanted this to look like?

fingolfin · 2016-01-18T18:11:21Z

lib/grp.gi

+
+    if not IsFinite(G) or not IsNilpotentGroup(G) then
+      TryNextMethod();
+    fi;


Won't this usually end up computing the size of the group? That in turn may not terminate. Perhaps have a CanComputeSize check first, perhaps as part of the filter?

An easy way to trigger a problem:

F:=FreeGroup(2);; G:=F/[F.1^2,F.2^2]; Socle(G);

You are right, it should also check first for CanComputeSize first.

Do you want a tst file on Socle, as well? At least for testing the nilpotent case?

Ok, I have added CanComputeSize and a tst file. Checks are running now.

fingolfin · 2016-01-18T18:14:04Z

This looks basically fine to me now, but perhaps it could / should be squashed into a single commit?

@hulpke any remaining concerns?

fingolfin · 2016-01-20T15:16:30Z

Perhaps squash this into 1-2 commits?

Thanks also for the test case -- perhaps I could persuade you to make one tiny change: Move the file to tst/testinstall/opers/Socle.tst (also note the case) -- the idea being that we collect files with tests specific to a single function in a uniform way. It would then match what I did in #400.

But that's not absolutely necessary.

This commit adds a method of computing the Socle for finite nilpotent groups, computing it as the Omega-parts of the Center. This method is ranked as high as if it were for only finite nilpotent groups, Therefore this method will run even if the group does not yet know whether it is a nilpotent group or not. This method forces an IsNilpotent check after a CanComputeSize and an IsFinite check. A sidenote: it would be possible to compute the p-parts first, and then take the center. However, for big permutation groups it is faster to compute the SylowSystem of the Center of the group, than the other way around. For polycyclic presentations there does not seem to be any difference, though.

hungaborhorvath · 2016-01-21T01:39:16Z

Squashed it into 2 commits, and moved the tst file to the requested directory. Running checks now.

…roups Add method for Socle for finite nilpotent groups

olexandr-konovalov · 2016-01-23T00:31:44Z

@hungaborhorvath thank you for adding Socle.tst file. We run it in two configurations to check that it works with no packages loaded (except needed by GAP) and with all packages loaded, to check that none of the packages breaks the test.

With all packages loaded, the test fails because of the diffs shown below. I suggest to modify the test in a way that it does not depend on particular generators of the group. For example, you may suppress the output with double semicolon, and then use Size or StructureDescription or IdGroup etc. to verify properties of the group.

########> Diff in /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGM\
P/nogmp/GAPTARGET/install/label/graupius/GAP-git-snapshot/tst/testinstall/oper\
s/Socle.tst, line 2:
# Input is:
Socle(DihedralGroup(8));
# Expected output:
Group([ f3 ])
# But found:
<pc group with 1 generators>
########
########> Diff in /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGM\
P/nogmp/GAPTARGET/install/label/graupius/GAP-git-snapshot/tst/testinstall/oper\
s/Socle.tst, line 8:
# Input is:
Socle(DirectProduct(D, D, D));
# Expected output:
Group([ (1,3)(2,4), (5,7)(6,8), (9,11)(10,12) ])
# But found:
Group([ (9,11)(10,12), (5,7)(6,8), (1,3)(2,4) ])
########
########> Diff in /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGM\
P/nogmp/GAPTARGET/install/label/graupius/GAP-git-snapshot/tst/testinstall/oper\
s/Socle.tst, line 10:
# Input is:
Socle(QuaternionGroup(8));
# Expected output:
Group([ y2 ])
# But found:
<pc group with 1 generators>
########
########> Diff in /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGM\
P/nogmp/GAPTARGET/install/label/graupius/GAP-git-snapshot/tst/testinstall/oper\
s/Socle.tst, line 12:
# Input is:
Socle(SymmetricGroup(4));
# Expected output:
Group([ (1,4)(2,3), (1,2)(3,4) ])
# But found:
Group([ (1,4)(2,3), (1,3)(2,4) ])
########

hungaborhorvath · 2016-01-23T21:05:40Z

Ok, I have restored the branch in my github account and I will rewrite the tests. Should I squash it into the last commit (Add tst/testinstall/opers/Socle.tst) or should I create a new commit with the changes? Further, should I do a new pull request or is it enough if I do the change on this branch?

hungaborhorvath · 2016-01-24T14:01:16Z

Ok, I have decided to do a new PR for the test file, see #530

hungaborhorvath mentioned this pull request Dec 14, 2015

Socle takes a long time to calculate for nilpotent (even for abelian) groups #385

Closed

fingolfin reviewed Dec 14, 2015
View reviewed changes

fingolfin reviewed Jan 18, 2016
View reviewed changes

hungaborhorvath added 2 commits January 21, 2016 02:30

Add tst/testinstall/opers/Socle.tst

8c6d6cf

hungaborhorvath force-pushed the SocleForFiniteNilpotentGroups branch from c6e5e5a to 8c6d6cf Compare January 21, 2016 01:38

fingolfin added a commit that referenced this pull request Jan 21, 2016

Merge pull request #402 from hungaborhorvath/SocleForFiniteNilpotentG…

6b129de

…roups Add method for Socle for finite nilpotent groups

fingolfin merged commit 6b129de into gap-system:master Jan 21, 2016

hungaborhorvath deleted the SocleForFiniteNilpotentGroups branch January 21, 2016 23:00

hungaborhorvath restored the SocleForFiniteNilpotentGroups branch January 23, 2016 20:58

hungaborhorvath deleted the SocleForFiniteNilpotentGroups branch January 24, 2016 13:12

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method for Socle for finite nilpotent groups #402

Add method for Socle for finite nilpotent groups #402

hungaborhorvath commented Dec 14, 2015

fingolfin Dec 14, 2015

hungaborhorvath Dec 14, 2015

fingolfin Dec 14, 2015

hulpke Dec 14, 2015

now socle is product of Omega of the center of its Sylow subgroups

fingolfin Dec 15, 2015

hungaborhorvath commented Dec 14, 2015

hungaborhorvath commented Dec 14, 2015

hungaborhorvath commented Dec 16, 2015

fingolfin commented Dec 16, 2015

hungaborhorvath commented Dec 16, 2015

stevelinton commented Dec 16, 2015

hungaborhorvath commented Dec 19, 2015

hungaborhorvath commented Dec 20, 2015

fingolfin Jan 18, 2016

hungaborhorvath Jan 19, 2016

hungaborhorvath Jan 20, 2016

fingolfin commented Jan 18, 2016

fingolfin commented Jan 20, 2016

hungaborhorvath commented Jan 21, 2016

olexandr-konovalov commented Jan 23, 2016

hungaborhorvath commented Jan 23, 2016

hungaborhorvath commented Jan 24, 2016

Add method for Socle for finite nilpotent groups #402

Add method for Socle for finite nilpotent groups #402

Conversation

hungaborhorvath commented Dec 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

now socle is product of Omega of the center of its Sylow subgroups

Choose a reason for hiding this comment

hungaborhorvath commented Dec 14, 2015

hungaborhorvath commented Dec 14, 2015

hungaborhorvath commented Dec 16, 2015

fingolfin commented Dec 16, 2015

hungaborhorvath commented Dec 16, 2015

stevelinton commented Dec 16, 2015

hungaborhorvath commented Dec 19, 2015

hungaborhorvath commented Dec 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jan 18, 2016

fingolfin commented Jan 20, 2016

hungaborhorvath commented Jan 21, 2016

olexandr-konovalov commented Jan 23, 2016

hungaborhorvath commented Jan 23, 2016

hungaborhorvath commented Jan 24, 2016