Speed improvements for automorphism groups #2383
Conversation
hulpke
added
kind: enhancement
|
Typo in commit message: shuitable |
| @@ -18,34 +18,101 @@ MORPHEUSELMS := 50000; | |||
|
|
|||
| InstallMethod(Order,"for automorphisms",true,[IsGroupHomomorphism],0, | |||
| function(hom) | |||
| local map,phi,o,lo,i,start,img; | |||
| local map,phi,o,lo,i,j,start,img,d,nat,ser,jord,first; | |||
| d:=Source(hom); | |||
There was a problem hiding this comment.
A short comment here as to what this method does would be very helpful. Right now, I see the commit message, which heelps, but if I read that code in a year, I won't know about it... BTW the commit message says a "characteristic series" is computed, but in some cases it only checks if the series is invariant under hom (which of course is enough)
So, wow about this
#To compute the order, use a suitable characteristic series 'ser' and work in the factor groups.
(And perhaps "characteristic series" should be "series invariant under 'hom'" ?
lib/morpheus.gi
Outdated
| lo:=lo+1; | ||
|
|
||
| # do the bijectivity test only if high local order, then it does not | ||
| # matter. IsBiojective is cached, sop second test is cheap. |
lib/morpheus.gi
Outdated
| lo:=lo+1; | ||
|
|
||
| # do the bijectivity test only if high local order, then it does not | ||
| # matter. IsBiojective is cached, sop second test is cheap. |
Codecov Report
@@ Coverage Diff @@
## master #2383 +/- ##
==========================================
+ Coverage 74.34% 74.34% +<.01%
==========================================
Files 481 481
Lines 243429 243574 +145
==========================================
+ Hits 180968 181084 +116
- Misses 62461 62490 +29
|
|
@alex-konovalov |
|
@alex-konovalov Why do we need to backport this to 4.9? I am not convinced, and very reluctant to do so. See my comment on PR #2377 for details |
|
@fingolfin your reluctance definitely has some point. If this breaks manual examples, it may also break tests in packages. If we will decide to backport, then only after we will check which impact that may have on packages. If we do not backport, then we need to decide whether to release with #2377 unresolved or not. |
lib/morpheus.gi
Outdated
| # this method calculates a chief series invariant under `hom` and calculates | ||
| # orders of group elements in factors of this series under action of `hom`. | ||
| # Every time an orbit length is found, `hom` is replaced by the appropriate | ||
| # power. Initiall small chief factors are preferred. In the end all |
|
@fingolfin Thanks. I will wait until #2377 is in some way or another sorted out. |
hulpke
removed
the
do not merge
by working in suitable hom-stable factors first.
Includes improvements to `SmallGeneratingSet` for perm groups. Also dealt with changed generators in manual examples
fingolfin
added
the
release notes: added
Improvements to element order, to
SmallGeneratingSetfor large degree permutation groups (that come up in the automorphisms groups code) and for isomorphism test in the case of solvable automorphism groups.(This makes the issue in #2377 disappear)