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 FrattiniSubgroup for nilpotent groups #583

Merged
merged 3 commits into from
Mar 28, 2016

Conversation

hungaborhorvath
Copy link
Contributor

General FrattiniSubgroup method is added for nilpotent groups. Now this
works for fp-groups, as well, see test examples.
Further, the nilpotent permutation p-group case is enhanced (no factorization is needed, only checking if a group is a p-group). I am not entirely sure if this permgroup method here is really needed, though, because this new general method for any nilpotent group should really cover it.

Tests are added.

The test file may conflict with #400 after that has been merged. Further, currently IsNilpotent is not really good with nilpotent fp-groups, because in computing LowerCentralSeries checking if two groups are equal might not terminate.

@hungaborhorvath
Copy link
Contributor Author

Can someone help me how to merge current master and rebase this so I can resolve the conflict in the tst file? Sorry about this.

General FrattiniSubgroup method is added for nilpotent groups. Now this
works for fp-groups, as well, see test examples.
Further, the nilpotent permutation p-group case is enhanced.

Tests are added.
@hungaborhorvath
Copy link
Contributor Author

I tried an experimental rebase master, and hopefully, resolved the conflict.

if q<>0 and not IsPrime(q) then
p := SmallestRootInt(q);
Add(gen, IndependentGeneratorsOfAbelianGroup(G)[i]^p);
fi;
Copy link
Member

Choose a reason for hiding this comment

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

Calling AbelianInvariants and IndependentGeneratorsOfAbelianGroup again and again like that makes me cringe. While we cache these values, each of these calls incurs an overhead, as the system has to perform a lookup in a component object. So please store those into temporary variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fingolfin Ok, now I call them in the beginning and then use the elements of this stored list.
The reason I did not want to do this was that if an abelian group has only invariants of 0 and prime then we would not even need to call IndependentGenerators, which could be more expensive than AbelianInvariants.

@fingolfin
Copy link
Member

Except for that minor remark, this looks good to me. Thanks!

Though I am not an expert on that code -- as always, I'd feel more comfortable if @hulpke had a brief look, too (if his time permits). And as always, of course everybody else is most welcome to take a look, too (the more, the better ;-).

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Feb 10, 2016
@hungaborhorvath
Copy link
Contributor Author

@fingolfin Sorry, I forgot to add the theory I used in the first post. Here it goes:

For a nilpotent group every maximal subgroup is normal. Thus the factor group exists and is a group without any proper subgroups, i.e. a cyclic group of prime power order. Hence every maximal subgroup contains the commutator and thus it is enough to consider G/G', which is abelian.

The Frattini subgroup of a direct product is the direct product of the Frattini subgroups. (Hm, a method probably should be implemented for direct products, as well... :-) ) Thus one only needs to consider cyclic groups. For the infinite group

is maximal for every prime, their intersection is 0. For a cyclic group of order p^n there is a unique maximal subgoup of order p^{n-1}.

Just before merging I will squash the commits if you want and then add this as comment to document it.

@hungaborhorvath
Copy link
Contributor Author

BTW, I did not make any checks if this method is any faster for pc-groups than the currently installed ones. It really should be (and then it should be ranked higher), but I only checked it in a few examples where this method was in fact slower.

On the non-commutative side, constructing CommutatorFactorGroup takes a lot of time, I really do not have any idea why.... On the commutative part, an insane amount of time is spent on IndependentGeneratorsOfAbelianGroup for e.g. a very big (100!-element) cyclic group, while the PreFrattini method works like a charm in seconds. This was mentioned already by @bh11 in #592. In both of these cases the PreFrattini method is called first, which is fine.

So probably the rank should stay as it is for now.

@hulpke
Copy link
Contributor

hulpke commented Feb 11, 2016

I really don't like the idea of installing lots of high-ranking methods that force-test properties in the hope that there will bring practical improvements. This might work OK for the examples studies by the author,
but I can imagine situations (say groups represented by a composition tree) where these tests add extra work. (On the other hand Fitting and Frattini Subgroup are not core functionality that much builds on.)

If a user suspects that properties should be set but are not because of particular constructions/representations, it always would be possible to just set the filters. I therefore would strongly prefer if this method was installed properly for the IsNilpotentGroup filter without extra ranking.

In the code, instead of calling CommutatorFactorGroup and then taking its natural homomorphism, I think it would be cleaner to use NaturalHomomorphismByNormalSubgroup(NC) in the first place, but that might be personal preference.

@hungaborhorvath
Copy link
Contributor Author

@hulpke Would you prefer to install this method for nilpotent groups and then have a Redispatch? Considering that for infinite groups the only other working method I see is for pcp-groups, it would warrant a redispatch, but from some of your comments I thought that you do not like it.

I will look if MaximalAbelianQuotient could be used instead of CommutatorFactorGroup, which you wrote in the other thread. This in fact calls for NaturalHomomorphismByNormalSubgroupNC for non-fp-groups.

@hungaborhorvath
Copy link
Contributor Author

@hulpke I have replacedCommutatorFactorGroup by MaximalAbelianQuotient.

The only method currently ranked lower than this are at maxsub.gi:335 and grp.gi:1047. If you honestly think that any of these methods would receive a significant hindrance by computing IsAbelian and IsNilpotentGroup first and/or they would perform for any nilpotent group better than this method, then I will write this for nilpotent groups with a redispatch.

Note, however, that in that case this method will never run, except when the group does know that it is nilpotent. For example for Abelian fp-groups (for which the group does not yet know it is Abelian) the algorithm would never run, because errors out during the radical method. Or, even worse, for infinite pcp-groups (that method ranked higher currently) FrattiniSubgroup errors out with no algorithm available, instead of the usual TryNextMethod() In summary, practically there would still be no method for infinite groups, and even the redispatch would not run, because the algorithm would error out in a previous method.

@markuspf markuspf merged commit 2b0f3c7 into gap-system:master Mar 28, 2016
@hungaborhorvath hungaborhorvath deleted the FrattiniSubgroup branch April 3, 2016 19:31
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants