Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method for Socle for finite nilpotent groups #402

Merged

Conversation

hungaborhorvath
Copy link
Contributor

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?

SetCentralizerInParent(prodH, Parent(prodH));
SetIsNormalInParent(prodH, true);
fi;
od;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hungaborhorvath
Copy link
Contributor Author

Added RedispatchOnCondition checking for finiteness and nilpotency.

@hungaborhorvath
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

There is BrowseGapMethods() in the browse package.

@hungaborhorvath
Copy link
Contributor Author

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
Copy link
Contributor Author

@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?


if not IsFinite(G) or not IsNilpotentGroup(G) then
TryNextMethod();
fi;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fingolfin
Copy link
Member

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

@hulpke any remaining concerns?

@fingolfin
Copy link
Member

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
Copy link
Contributor Author

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

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

Add method for Socle for finite nilpotent groups
@fingolfin fingolfin merged commit 6b129de into gap-system:master Jan 21, 2016
@hungaborhorvath hungaborhorvath deleted the SocleForFiniteNilpotentGroups branch January 21, 2016 23:00
@olexandr-konovalov
Copy link
Member

@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 hungaborhorvath restored the SocleForFiniteNilpotentGroups branch January 23, 2016 20:58
@hungaborhorvath
Copy link
Contributor Author

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 hungaborhorvath deleted the SocleForFiniteNilpotentGroups branch January 24, 2016 13:12
@hungaborhorvath
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants