-
Notifications
You must be signed in to change notification settings - Fork 160
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
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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], |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
.
BTW, It might be better to instead modify |
@ChrisJefferson, you're right. Amended. |
dd2f34f
to
05600a8
Compare
Codecov Report
@@ 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
|
In response to @fingolfin's comments, I've gone for I don't know anything about |
If a group knows randomised recognition I can only find in Adding an additional case to |
@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. |
There was a problem hiding this 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.
@fingolfin, I was ignoring "trivial" cases. Let's wait for @hulpke to comment. |
Observation: one could simply install |
(Well, almost: the trivial group needs to be added in the nonsolvable cases) |
There was a problem hiding this 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?
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:
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) |
Ok, now I see why one would want to integrate this into the 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 Incidentally, @hulpke, is here a reference/publication that describes how |
05600a8
to
4f70203
Compare
I've updated this to use In order to have this method selected over the method for @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);; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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...
4f70203
to
43aecbd
Compare
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 Anyway: tests are failing because the changes here affect the example in the manual, in |
43aecbd
to
efd5eca
Compare
@markuspf |
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.