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

Enhance MaximalNormalSubgroups for finite solvable groups #552

Merged
merged 8 commits into from
Mar 28, 2016

Conversation

hungaborhorvath
Copy link
Contributor

For finite solvable groups every maximal normal subgroup contains G', thus we can factor out with this.
For abelian groups, a maximal normal subgroup looks like a maximal normal subgroup in one Sylow multiplied by all other Sylows.
For an abelian p-group maximal normal subgroups are the same as maximal subgroups, hence we can factor out by the Frattini subgroup.
For elementary abelian groups one can use NormalMaximalSubgroups from grppcatr.gi. This was the main reason I put this method here rather than into grp.gi.
MaximalSubgroups could also be used, but seems to do some extra checks which are unnecessary for elementary abelian groups.

Rank of this method is raised to become RankFilter(CanComputeSize and IsFinite and IsSolvable).

Some parts probably can be generalized for polycyclic groups, but I did not check thoroughly.

Added tests, profiling says that all relevant lines are run.

Some more testing shows that for big abelian or dihedral groups the new method is significantly faster. These are not added to the tst file. In any case, for solvable groups we spare a NormalSubgroups computation.

@hungaborhorvath
Copy link
Contributor Author

I personally do not like that MaximalNormalSubgroups returns a list rather than a set. Nevertheless, I went with this convention in the new method.

Is there any particular reason why it is chosen to be not a set but a list?

@ChrisJefferson
Copy link
Contributor

Someone else may have a better answer, but I know in some cases it is because making a Set requires sorting, and some objects are very expensive to compare with '<'.

@markuspf
Copy link
Member

@ChrisJefferson that'd be the reason: Sets are implemented as sorted duplicate-free lists, which can become really expensive for objects like subgroups of groups. Anyone volunteering to work on datastructures yet ;)

@hungaborhorvath
Copy link
Contributor Author

#379 has a function UnionIfCanEasilySortElements, which essentially computes the union of the arguments, unless one of them has not CanEasilySortElements. I guess all of the appropriate functions corresponding to sets/lists could be rewritten, but it probably should be done in a unified way.

@markuspf
Copy link
Member

@hungaborhorvath implementations of Set is a can of worms, which we can discuss in a separate issue, there are questions like "when are things in a set considered equal" and "how expensive is it to determine equality" involved.

@hulpke
Copy link
Contributor

hulpke commented Jan 29, 2016

What `MaximalNormalSubgroups' (and similar functions) return is a duplicate free list. For all practical purposes this behaves like a mathematical set.

It is not a Set in GAP-sense, since comparison of subgroups (which we guarantee to use the < operator and to be compatible with a lex-comparison of the element sets) is comparatively expensive.
CanEasilySortArguments is a red herring here -- it is not set for sets of subgroups, I think.

What would be the benefit of making it a Set?`

@hungaborhorvath
Copy link
Contributor Author

@hulpke Hm, I see. So Set in GAP is not the same as a set in mathematics, and there it is guaranteed that it is sorted. So it is easier to append to it, and easier to search in it. That I would call an advantage, but only if it does not take much time to sort the new element into the existing set.

So the consensus is that it should remain a list, in all situations?

@hulpke
Copy link
Contributor

hulpke commented Jan 29, 2016

All similar functions return lists, not sets.

@@ -0,0 +1,21 @@
gap> START_TEST("normal_hall_subgroups.tst");
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the argument be "MaximalNormalSubgroups.tst"?

@hungaborhorvath
Copy link
Contributor Author

@alex-konovalov Of course it should. I copied another (WIP) tst file and modified it, obviously left the name unchanged.... Sorry.

@hungaborhorvath
Copy link
Contributor Author

This method can be turned with not too much effort to work for some infinite groups, as well. Would it make sense to do such modifications to this PR?

New MaximalNormalSubgroups is rewritten to work for infinite groups, as well.
If G/G' is infinite, then there are infinitely many maximal normal subgroups,
and the method errors out. If G is nonabelian, then maximal normal subgroups
of G/G' are pulled back. If G is Abelian, then first it is represented as a
pc-group, then NormalMaximalSubgroups are computed, then pulled back to the
original group.

The tst file is also rewritten to test some infinite groups.
@hungaborhorvath
Copy link
Contributor Author

Ok, the new MaximalNormalSubgroups is rewritten to work for infinite (solvable) groups, as well.
If G/G' is infinite, then there are infinitely many maximal normal subgroups,
and the method errors out. If G is nonabelian, then maximal normal subgroups
of G/G' are pulled back. If G is Abelian, then first it is represented as a
pc-group, then NormalMaximalSubgroups are computed and pulled back to the
original group.

The tst file is also rewritten to test some infinite groups.

Finally, let me mention that the tst file has some commented lines. These all run within 8 seconds on my laptop if I put them in line by line. However, when I run the tst file, then they do not finish even in one minute. Anybody has any idea why that is? Thank you.

@olexandr-konovalov
Copy link
Member

@hungaborhorvath Regarding the test, most likely it's the assertion level. Try to change it with SetAssertionLevel(2) in the command line and you will likely see the same speed as in the test.

There is a related discussion on assertion levels here: #527, #549, #564.

@hungaborhorvath
Copy link
Contributor Author

@alex-konovalov Thank you. I have updated the test file to run in reasonable time (10 seconds on my laptop).

hom, # homomorphism from G to Gf
MaxGf; # MaximalNormalSubgroups of Gf

if 0 in AbelianInvariants(G) then
Copy link
Member

Choose a reason for hiding this comment

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

So you method is actually called for arbitrary groups (not just solvable ones), and you assume that you can compute AbelianInvaits, IsAbelian, IsSolvableGroup "cheaply". Hmm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well..... yes. My gut says that if for a group we cannot even compute AbelianInvariants, IsAbelian and IsSolvableGroup cheaply, then we have little chance of computing the maximal normal subgroups cheaply by any other method. But I did not verify this feeling in any way.

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Feb 10, 2016
if not IsPcGroup(G) then
# convert it to an Abelian PcGroup with same invariants
Gf := AbelianGroup(IsPcGroup, AbelianInvariants(G));
hom := IsomorphismGroups(G, Gf);
Copy link
Contributor

Choose a reason for hiding this comment

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

IsomorphismGroups is potentially expensive -- Why construct a new group and not work in the old one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hulpke NormalMaximalSubgroups needs CanEasilyComputePcgs. I would think that finite Abelian groups should get this, but currently they do not:

gap> F := FreeGroup("x", "y", "z");;
gap> x := F.1;; y := F.2;; z := F.3;;
gap> G := F/[x^(-1)*y^(-1)*x*y, x^(-1)*z^(-1)*x*z, z^(-1)*y^(-1)*z*y, (x*y)^180, (x*y^5)^168, z];
<fp group on the generators [ x, y, z ]>
gap> IsAbelian(G); time; 
true
4
gap> AbelianInvariants(G); time;
[ 3, 4, 5, 7, 9, 32 ]
4
gap> HasSize(G);
true
gap> CanEasilyComputePcgs(G); 
false
gap> NormalMaximalSubgroups(G);; time;
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 2nd choice method found for `NormalMaximalSubgroups' on 1 arguments called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at line 24 of *stdin*
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> 

On the other hand, IsomorphismGroups at least finishes, though really slowly....

gap> Gf := AbelianGroup(IsPcGroup, AbelianInvariants(G));; time; 
4
gap> hom := IsomorphismGroups(G, Gf);; time;
80
gap> MaxGf := NormalMaximalSubgroups(Gf);; time;
24
gap> List(MaxGf, N -> PreImage(hom, N));; time;
15256

I would prefer not to use isomorphism (or do it with NiceMonomorphism, but I do not understand properly their implementations), but that would need some work on the Abelian groups that you collected in #596

Copy link
Contributor

Choose a reason for hiding this comment

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

The whole purpose of CanEasilyComputePcgs is that a group might know to be solvable (say because a flag has been set by the user) but there still is no good method for computing a PCGS. FpGroup are exactly of this form, unless an isomorphism to a solvable group has already been constructed.

It is not a good idea to use expensive functions (such as potentially IsomorphismGroups) just because the context is not clear (and no -- this is not really what the NiceMonomorophisms are intended for).
For FpGroups there is for example MaximalAbelianQuotient which is probably as efficient as one could hope for.

I'm slightly confused by NormalMaximalSubgroups. Aren't we talking about MaximalNormalSubgroups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hulpke NormalMaximalSubgroups computes all maximal subgroups that are actually normal. In abelian groups they are the same as MaximalNormalSubgroups, and also the same as MaximalSubgroups. The code of the latter looked to me exactly the same as NormalMaximalSubgroups except that some extra checks are done in that one (which are unnecessary in the abelian case). That is why I used NormalMaximalSubgroups, instead.

Would you prefer that I wrote a general algorithm for abelian groups using independent generators and drop the whole isomorphism route? (I did not want to do that because an algorithm exists already, but if you think that would be the proper way, I will do that.)

BTW, again I can only stress that some of these algorithms are incredibly slow for abelian groups. After having independent generators, nothing should be slow anymore, not even isomorphisms....

About MaximalAbelianQuotient: hm, looking at the code it looks like this is the one to go for all the time, and not CommutatorFactorGroup. However, I am now slightly confused about why CommutatorFactorGroup exists at all.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced CommutatorFactorGroup by MaximalAbelianQuotient

@hungaborhorvath
Copy link
Contributor Author

BTW, Is it just me or the checking part of this page is really gone?

@markuspf markuspf added this to the GAP 4.9.0 milestone Mar 7, 2016
@markuspf markuspf merged commit 3488e16 into gap-system:master Mar 28, 2016
@olexandr-konovalov
Copy link
Member

@hungaborhorvath Since this PR now merged, could you please add to the Wiki page for the next major release the text to include in the changes manual and the release announcement?

@hungaborhorvath
Copy link
Contributor Author

@alex-konovalov I wrote a few sentences, does it look fine?

@hungaborhorvath hungaborhorvath deleted the MaximalNormalSubgroups branch April 3, 2016 19:32
fingolfin added a commit that referenced this pull request Nov 9, 2016
Fix some issues with MaximalNormalSubgroups introduced from #692 after
merging #552. 

- NormalSubgroup computation method is only called for finite groups.
- Redispatch is added for two methods (finite, solvable). (Not added for
  abelian, because abelian groups should be recognized by the IsSolvabe
  test.)
- New generic method added checking if AbelianInvariants has 0.
- Abelian method checks if AbelianInvariants has 0 (for non pc-groups).
- If GAP finds that there are infinitely many maximal normal subgroups,
  then it returns fail instead of erroring out (e.g. if 0 is in
  AbelianInvariants).
- Manual is changed to reflect this new behaviour, an example is added,
  as well.
- Lists are replaced by sorted lists in test file and some more tests
  are added.
@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.

6 participants