-
Notifications
You must be signed in to change notification settings - Fork 162
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
Minor performance improvements, code cleanup and very local fixes #2733
Changes from all commits
04c80d8
4aa1666
92cb1a8
a8f8e61
82eb95e
2f92e5d
41fe45c
14157d1
d9b2efc
3c243da
2d2a61a
474a064
f4ede03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1519,7 +1519,7 @@ end ); | |
## | ||
#M Socle( <G> ) . . . . . . . . . . . socle of primitive permutation group | ||
## | ||
InstallMethod( Socle,"for permgrp", true, [ IsPermGroup ], 0, | ||
InstallMethod( Socle,"test primitive", true, [ IsPermGroup ], 0, | ||
function( G ) | ||
local Omega, deg, shortcut, coll, d, m, c, ds, L, z, ord, | ||
p, i; | ||
|
@@ -1539,25 +1539,27 @@ InstallMethod( Socle,"for permgrp", true, [ IsPermGroup ], 0, | |
deg := Length( Omega ); | ||
if deg >= 6103515625 then | ||
TryNextMethod(); | ||
elif deg < 12960000 then | ||
shortcut := true; | ||
if deg >= 3125 then | ||
coll := Collected( Factors(Integers, deg ) ); | ||
d := Gcd( List( coll, c -> c[ 2 ] ) ); | ||
if d mod 5 = 0 then | ||
m := 1; | ||
for c in coll do | ||
m := m * c[ 1 ] ^ ( c[ 2 ] / d ); | ||
od; | ||
if m >= 5 then | ||
shortcut := false; | ||
fi; | ||
fi; | ||
fi; | ||
if shortcut then | ||
ds := DerivedSeriesOfGroup( G ); | ||
return ds[ Length( ds ) ]; | ||
fi; | ||
# the normal closure actually seems to be faster than derived series, so | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Maybe] Remove the code instead of commenting it out, and describe in the comment that formerly there was some code here which was removed because of some reasons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I have run or analyzed enough examples to state categorically that the old (now commented out) code is clearly useless. Someone (well, myself) might stumble on this place again when looking at an inefficiency, and it might turn out that under certain circumstances the now commented-out code is in fact better. If it is immediately removed, it is hard to recover it from git. |
||
# this is not really a shortcut | ||
#elif deg < 12960000 then | ||
# shortcut := true; | ||
# if deg >= 3125 then | ||
# coll := Collected( Factors( deg ) ); | ||
# d := Gcd( List( coll, c -> c[ 2 ] ) ); | ||
# if d mod 5 = 0 then | ||
# m := 1; | ||
# for c in coll do | ||
# m := m * c[ 1 ] ^ ( c[ 2 ] / d ); | ||
# od; | ||
# if m >= 5 then | ||
# shortcut := false; | ||
# fi; | ||
# fi; | ||
# fi; | ||
# if shortcut then | ||
# ds := DerivedSeriesOfGroup( G ); | ||
# return ds[ Length( ds ) ]; | ||
# fi; | ||
fi; | ||
|
||
coll := Collected( Factors(Integers, Size( G ) ) ); | ||
|
@@ -1577,7 +1579,10 @@ InstallMethod( Socle,"for permgrp", true, [ IsPermGroup ], 0, | |
until ord mod p = 0; | ||
z := z ^ ( ord / p ); | ||
until Index( G, Centralizer( G, z ) ) mod p <> 0; | ||
L := NormalClosure( G, SubgroupNC( G, [ z ] ) ); | ||
|
||
# immediately add conjugate as this will be needed anyhow and seems to | ||
# speed up the closure process | ||
L := NormalClosure( G, SubgroupNC( G, [ z,z^Random(G) ] ) ); | ||
if deg >= 78125 then | ||
ds := DerivedSeriesOfGroup( L ); | ||
L := ds[ Length( ds ) ]; | ||
|
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.
[Question] would it be useful to document this attribute?
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.
My concern was that this could lead to confusion as$G/G'$ .
AbelianInvariants
will (in general) return a longer list by separating different primes, while this attribute gives the minimal generator number of