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 NormalSubgroups methods for symmetric and alternating permutation groups #2684

Merged
merged 2 commits into from
Aug 8, 2018

Conversation

mtorpey
Copy link
Contributor

@mtorpey mtorpey commented Aug 6, 2018

The normal subgroups of a symmetric group are well-known and simply described: the trivial group, the Klein 4-group (if n=4), the alternating group, and the symmetric group itself. An alternating group's normal subgroups are even simpler: the same but without the symmetric group. The NormalSubgroups attribute in GAP should reflect this knowledge by having special methods for permutation groups that are known to be symmetric or alternating.

At the moment, some laborious work is done which means that calling, for example, NormalSubgroups(SymmetricGroup(40)), takes several seconds, while larger examples run for a very long time. This PR introduces simple methods that return the appropriate subgroups.

Note that this only applies to permutation groups, since it is easy to describe subgroups based on their moved points. Note also that this is not a fix for Issue #2683, a bug that probably applies to non-symmetric groups as well.

Copy link
Contributor

@ChrisJefferson ChrisJefferson left a comment

Choose a reason for hiding this comment

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

I think you might want isnaturalsymmetricgroup, not issymmetricgroup, but I am away from a computer to check properly

lib/gpprmsya.gi Outdated
local pts, list;
pts := MovedPoints(G);
list := [Group(())]; # Trivial group
if Length(pts) = 4 then # Klein 4-group
Copy link
Member

Choose a reason for hiding this comment

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

This check is only valid if the symmetric group is in its "natural" form, as tested by IsNaturalSymmetricGroup. But Sym(4) also has e.g. a permutation representation acting on 24 points (i.e. the regular module):

gap> G:=Image(RegularActionHomomorphism(SymmetricGroup(4)));;
gap> IsSymmetricGroup(G);
true
gap> IsNaturalSymmetricGroup(G);
false
gap> NrMovedPoints(G);
24

lib/gpprmsya.gi Outdated

InstallMethod(NormalSubgroups,
"for a perm group isomorphic to the alternating group",
[IsPermGroup and IsAlternatingGroup],
Copy link
Member

Choose a reason for hiding this comment

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

I guess something similar could also be installed for IsSimpleGroup... Not saying you should, just thinking out loud. The case with few moved points are mostly trivial to compute anyway.

lib/gpprmsya.gi Outdated
local pts, list;
pts := MovedPoints(G);
list := [Group(())];
if Length(pts) = 4 then
Copy link
Member

Choose a reason for hiding this comment

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

Again, this test only is sufficient if you restrict to IsNaturalAlternatingGroup. Or switch the condition of this method to IsSimpleGroup.

@fingolfin
Copy link
Member

BTW, It might be better to instead modify NormalSubgroupsCalc to teach it about these cases. Probably @hulpke has something to say about that, too... My personal impression here is that this is something where group recognition would help for the full "proper" solution, but @hulpke will know far better.

@mtorpey
Copy link
Contributor Author

mtorpey commented Aug 7, 2018

@ChrisJefferson, you're right. Amended.

@codecov
Copy link

codecov bot commented Aug 7, 2018

Codecov Report

Merging #2684 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2684      +/-   ##
==========================================
- Coverage   75.41%   75.41%   -0.01%     
==========================================
  Files         478      478              
  Lines      241590   241598       +8     
==========================================
+ Hits       182201   182207       +6     
- Misses      59389    59391       +2
Impacted Files Coverage Δ
lib/gpprmsya.gi 67.23% <100%> (+0.26%) ⬆️
hpcgap/lib/hpc/stdtasks.g 64.19% <0%> (-0.52%) ⬇️
src/hpc/threadapi.c 43.48% <0%> (-0.11%) ⬇️

@mtorpey
Copy link
Contributor Author

mtorpey commented Aug 7, 2018

In response to @fingolfin's comments, I've gone for IsNaturalSymmetric/AlternatingGroup as the filter since, as he points out, this algorithm only applies to groups that are the symmetric group on their set of moved points. I've also made the tests less display-focused, which should allow them to pass regardless of which packages are loaded.

I don't know anything about NormalSubgroupsCalc, so I'll let others consider that. :)

@markuspf markuspf added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: performance bugs or enhancements related to performance (improvements or regressions) topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 7, 2018
@markuspf markuspf requested a review from hulpke August 7, 2018 10:23
@markuspf
Copy link
Member

markuspf commented Aug 7, 2018

If a group knows IsSymmetric, using DerivedSubgroup as the one proper normal subgroup would suffice (indeed to compute IsSymmetric, GAP computes the derived subgroup of G),

randomised recognition I can only find in recog.

Adding an additional case to NormalSubgroupCalc seems to be a rather complicated fix to this fairly simple problem.

@fingolfin
Copy link
Member

@markuspf I am not sure I understand what you are trying to say with the first paragraph of your last comment. Note that Sym(4) has another normal subgroup besides it derived subgroup.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Fine by me, though I'd prefer if @hulpke had a look at this, too.

@markuspf
Copy link
Member

markuspf commented Aug 7, 2018

@fingolfin, I was ignoring "trivial" cases.

Let's wait for @hulpke to comment.

@fingolfin
Copy link
Member

Observation: one could simply install DerivedSeries as a method in all these cases (does not require "natural" either)

@fingolfin
Copy link
Member

(Well, almost: the trivial group needs to be added in the nonsolvable cases)

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

I know that we would like to have tests, but isn't this a rather gratuitous list of test examples (that in the end take time and space). Is it really needed to have this number of tests to ensure all possible quirks are checked?

@hulpke
Copy link
Contributor

hulpke commented Aug 7, 2018

Since there was a request for comment:

This code does what it says on the box: If a user asks for the normal subgroups of a symmetric group which knows that it is a natural symmetric group, the normal subgroups are written down. This will avoid the slowdown that was observed.

However with moderate effort, it would still be possible to construct similar examples that continue to behave badly:

  • A group that does not yet have IsNaturalSymmetricGroup set.

  • A symmetric group in another permutation representation.

  • Another almost simple group.

I think it would not be hard to catch this last case generically in the algorithm, but of course this will never be as fast as a dedicated routine (and will not conflict with the proposed code)

@markuspf
Copy link
Member

markuspf commented Aug 7, 2018

Ok, now I see why one would want to integrate this into the NormalSubgroupsCalc: It'd work for any permutation group, even if it doesn't know its natural symmetric (or with improvements isomorphic to a symmetric group).

I think generalising the proposed code to at least work for any group that knows its symmetric is a fairly good thing to do (as @hulpke says this code wouldn't interfere with more sophisticated code that works for examples that don't even know that they're symmetric)

Furthermore, the test for IsSymmetric seems fairly cheap, essentially computing the derived subgroup and excluding some pesky counterexamples in the case of n=6, but now I am probably veering towards what the code in NormalSubgroupsCalc would do anyway.

Incidentally, @hulpke, is here a reference/publication that describes how NormalSubgroupsCalc works? That function looks quite daunting at first glance.

@mtorpey
Copy link
Contributor Author

mtorpey commented Aug 8, 2018

I've updated this to use DerivedSeries and DerivedSubgroup, and therefore to work even for non-natural symmetric and alternating groups. For now I've left everything in special methods, since I don't know a lot about NormalSubgroupsCalc.

In order to have this method selected over the method for IsPermGroup (which redirects to NormalSubgroupsCalc), I've added RankFilter(IsPermGroup) to the methods' priority. Does this seem reasonable?

@hulpke: Perhaps I went a little overboard writing tests. I'll rein it in a bit in future! :)


# Non-natural symmetric groups
gap> S4 := Group((1,2,3,4), (1,2));;
gap> hom := ActionHomomorphism(g, Arrangements([1..4], 4), OnTuples);;
Copy link
Member

Choose a reason for hiding this comment

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

change g to S4 here (else the test fails when run on its own).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! It's always the little things...

@fingolfin
Copy link
Member

I actually think that the tests added here are very good, and wish we had similarly thorough tests for many more operands. In particular, they cover the cases where the set we act on is not [1..n] (which is often forgotten), deals with edge cases (here: small n) etc.. However, the whole test runs in 250ms on my laptop, which is indeed a bit long (I mean, it's not much, but if you do the same thing for 1000 functions, that turns into 4 minutes). Simple solution: drop the two tests for degree 100 (or replace them by, say, degree 15), then it goes down to 20 milliseconds. Sure, those cases were what motivated you originally, but if we wanted to check for performance regressions, we really need to create a dedicated test setup for that first (see also issue #260).

Anyway: tests are failing because the changes here affect the example in the manual, in lib/grp.gd:1870.

@markuspf markuspf added the topic: tests issues or PRs related to tests label Aug 8, 2018
@markuspf markuspf merged commit 786fe9e into gap-system:master Aug 8, 2018
@hulpke
Copy link
Contributor

hulpke commented Aug 8, 2018

@markuspf
The algorithm in NormalSubgroupsCalc is described in my 1998 ISSAC paper (
http://www.math.colostate.edu/~hulpke/paper/normalsub.pdf )

@mtorpey mtorpey deleted the normal-subgroups-sym-alt branch August 8, 2018 16:20
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Sep 23, 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 topic: library topic: performance bugs or enhancements related to performance (improvements or regressions) topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants