Enhance DirectFactorsOfGroup for faster StructureDescription

[hungaborhorvath]

The default DirectFactorsOfGroup attribute has undergone a serious
enhancement, without having proper documentation, yet.

• The Kayal-Nezhmetdinov algorithm is implemented with some tweaks under the
  attribute DirectFactorsOfGroupKN. From now on, it is referred to as the
  KN method.
  Some parts of the code is reused in the main DirectFactorsOfGroup
  attribute, but there are no direct calls to DirectFactorsOfGroupKN.
• The KN method has an algorithm for computing a direct complement to a
  normal subgroup. This is implemented in the operation
  ComplementNormalSubgroup. This particular operation (in theory) should
  work for infinite groups, as well.
• The main DirectFactorsOfGroup algorithm works as follows:
  • for Abelian groups computes the decomposition to cyclic groups of
    prime-power order,
    (for infinite Abelian groups the output is only a list of the direct
    factors instead of a set, because calling Set on the factors might
    not finish running in reasonable time;
    infinite non-Abelian groups are not handled, at all)
  • for nilpotent groups it calls itself on the Sylow subgroups,
  • checks several sufficient conditions for the group being direct
    indecomposable,
  • looks for Abelian cyclic components from the center of the group,
  • checks for more sufficient conditions for the group being direct
    indecomposable,
  • computes the normal subgroups and the minimal normal subgroups, and
    calls DirectFactorsOfGroupFromList.
• DirectFactorsOfGroupFromList is a more efficient version of the old
  factorization method. It essentially searches through the normal subgroups
  (2nd argument, which is a list) by size and looks for a complement from
  the same list. If it finds a complement then the first factor is direct
  indecomposable, the second is decomposed further using the same list
  filtered from the clearly noncomplemented normal subgroups.
  Trivial intersection is checked by IsTrivialNormalIntersectionInList,
  which checks if any of the minimal subgroups (3rd argument, which is a
  list) is contained in both normal subgroups. This is faster than computing
  the normal intersection and checking if that is trivial.
• IsTrivialNormalIntersection is implemented in cases where one knows more
  about the structure of the group.
• Cases where the group is already a direct product or already has
  NormalSubgroups computed are handled in separate methods.
• Some lines contained a space at their end, these spaces are all removed
  automatically by the text editor.

Tested on SmallGroups of order different than 512, 768, 1024, 1280, 1536,
1792, and on a couple hundred test permutation groups.

More times than not the new method was significantly (>200ms, >10%) faster
than the old method, in many cases because it immediately found the group
is direct indecomposable, rather than spending much time computing the
normal subgroups. In particular, the groups specifically mentioned in
issue #160 are factored almost instantly now.

The KN method seems to be drastically slower if the direct factors are
non-Abelian. Computing normal subgrups in such cases is much faster.
However, the implementation is kept for possible future enhancements.

[fingolfin]

Thanks for your submission, appreciated. I'll add comments to your code changes next, as part of reviewing this PR.

One immediate comment: You mention tests you performed. It would be good if you could add some tests to this PR, too.

I am also somewhat concerned about the method being drastically slower in certain cases. Can you provide a few concrete examples were things get noticably slower / faster?

[fingolfin]

The formatting here is off, lots of spaces are missing. E.g. this one should be

DeclareOperation( "IsTrivialNormalIntersection", [ IsGroup, IsGroup, IsGroup ] )

in order to fit in with the rest of the GAP library code.

[hungaborhorvath]

Sorry, I did not know about the GAP formatting protocol. Could you please point me to it? Thank you.

I have not yet had the chance to check GAPDoc, and I have no idea about how to make manuals, so if it explained there, then I certainly have not yet read it. I will gladly comply with it and make the code into the proper formatting after I know exactly what to do.

[hungaborhorvath]

I am not sure how to write replies, and how to use the github facilities properly, but I will try to do my best. In any case, thank you very much for all the comments and feedback.

About the algorithm: there are two separate things here. The KN-method is implemented, but I found that some parts are simply do not seem efficient. So I only incorporated those parts into the main algorithm that indeed seemed to compute the direct factors faster. The main example here is to look for a direct factor in the center. Further, I use some of my own heuristics in the main algorithm which seemed reasonable. Most of these are cheap but can drastically reduce computing time. In particular, the main method should_not be much slower than the other in any cases, because all checks that are done should be cheap (in some sense).

Tests: I have no idea how you would want me to put the tests here. I have a "work" branch in which I still have the original DirectFactorsOfGroup, and I simply ran through groups calculating the output and time the calculation took. Then with another set of commands I simply compared them. For the comparison I decided to consider the time difference significant if the difference was at least 200ms and at least 10%. On my own machine I usually only tested SmallGroups up to size 128, but on a supercomputer I did the rest. There are shell scripts that generate the input files, runs the commands, etc. If you tell me how you want me to upload them, I will gladly do it. (I want to rerun them now with Hulpke's fix in #376 but somehow jobs are not starting currently on the supercomputer I am using. (BTW, I use 1-core GAP, and do not use HPC-GAP at all)

Results of tests: (only some examples for now, can be made more precise if you want)
the examples from issue #160

gap> DirectFactorsOfGroupOld(SmallGroup(64,266));; time;
1244
gap> DirectFactorsOfGroup(SmallGroup(64,266));; time;
16

gap> DirectFactorsOfGroupOld(SmallGroup(128,2300));; time;
720
gap> DirectFactorsOfGroup(SmallGroup(128,2300));; time;
404

gap> DirectFactorsOfGroupOld(SmallGroup(128,2326));; time;
61512
gap> DirectFactorsOfGroup(SmallGroup(128,2326));; time;
8

gap> DirectFactorsOfGroupOld(SmallGroup(128,2327));; time;
64140
gap> DirectFactorsOfGroup(SmallGroup(128,2327));; time;
4

For SmallGroups of size 1-511 the new method was faster for

1 group with difference at least 1000 sec (not msec !)
5 groups with difference at least 100 sec
310 groups with difference at least 10 sec
1783 groups with difference at least 1 sec
(and these were at least 10% improvements, as well)

In comparison, there was 45 (!) groups where the old method was faster by at
least 1 sec, 11 of those that were faster with time between 2 and 3 sec, the
remaining 34 were faster with between 1 and 2 sec.

As for % faster, among these groups there were 2603 examples where the
difference was an about 10x faster algorithm, 259 of which was about 100x
faster. In comparison for the old method there were faster at least 10x for
24 groups, none of which was at least 100x faster, and for all of these it
was at most 3sec faster.

SmallGroup(128,5) is a typical example where the new method is slower than the old:

gap> DirectFactorsOfGroup(SmallGroup(128, 5));; time;
996
gap> DirectFactorsOfGroupOld(SmallGroup(128, 5));; time;
152

For big permuation groups, Stefan Kohl has a list. (BTW, he helped me a lot with all these, thank you.) The 18th of his examples was one where NormalSubgroups did not compute, because of the many complement problem. With the new method:

gap> tpg := ReadAsFunction("testpermgroups.g")();;
gap> G := tpg[18];
Group([ (1,2)(3,4)(5,6)(7,8)(9,10)(11,12)(13,14)(15,16)(17,18)(19,20)(21,22)
(23,24)(25,26)(27,28)(29,30), (2,3)(5,6)(8,9)(11,12)(14,15)(17,18)(20,21)
(23,24)(26,27)(29,30), (1,5)(6,10)(11,15)(16,20)(21,25)(26,30) ])
gap> DirectFactorsOfGroup(G); time;
[ Group([ (11,25,28,26,20,22)(12,27,29,14)(13,15,21,19,30,16), (11,22,26,20)
(12,28,27,25)(13,14,16,29)(15,21,30,19) ]), Group([
(4,6,7,5)(11,30)(12,29)
(13,28)(14,27)(15,26)(16,25)(17,18)(19,22)(20,21)(23,24), (1,6,9,3,7)
(2,8,4,10,5) ]), Group([ (1,10)(2,9)(3,8)(4,7)(5,6)(17,18)(23,24) ]) ]
35940
gap> HasNormalSubgroups(G);
false

For the old method I have this after several minutes:

gap> DirectFactorsOfGroupOld(G); time;
#I Many (2097152) complements!

Again, as I said, I will rerun the tests now, that Hulpke's fix is merged, and I will gladly run any other tests you want.

I will try to reply to the other comments within the comment if I can.

[hungaborhorvath]

Just added some documentation to the commands to make it more clear which one does what. I will apply the other suggestions later.

Can I ask for some help on how to separate the trailing whitespace removals from the other changes into a separate commit? Thank you.

[hungaborhorvath]

Commit 2ae9c90 enhances the main algorithm further. There is no theoretical enhancement, only coding.

I did testing with commit 2ae9c90 and compared to the old DirectFactorsOfGroup. All SmallGroups were tested except for orders 512, 768, 1024, 1280, 1536, 1792, 1920 and 873 test permutation groups were tested, as well. I checked whether any of the two methods was at least 200ms and 10% faster than the other. The old method for order 1152 groups have not yet finished.

On the SmallGroups, there were (altogether) 64 instances when the old method was faster, each of them is by less than a second. As a comparison, the current DirectFactorsOfGroup was faster
31221 many times for groups of order 1-511
5337 many times for groups of order 513-767
5176 many times for groups of order 769-1023
213 many times for groups of order 1023-1151
65920 many times for groups of order 1152
207 many times for groups of order 1153-1279
6338 many times for groups of order 1281-1535
24563 many times for groups of order 1537-1791
224 many times for groups of order 1793-1919
795 many times for groups of order 1921-2015

Here, in 30 instances the improvement was more than 1000 sec (!), e.g. SmallGroup(256, 56091) is a huge improvement by 2 full hours. In about 800 cases the improvement is between 100-1000 sec, in about 16000 cases the improvement is between 10-100 sec.

For the 873 test permutation groups the new method was faster 75 many times, the old method was faster 17 many times, 6 times by less than 1 second, 10 times by 1-10 seconds, and for one group the old method is faster by 80 seconds (this was a 30% decrease in speed, probably due to a bad choice of chief series). The new method was faster 3 times by about full 2 minutes, 23 times by 10-100 seconds, 20 times by 1-10 seconds, 29 times by less than 1 second.

Using
DirectFactorsOfGroupFromList(G, NormalSubgroups(G), MinimalNormalSubgroups(G));
runs almost always faster than the old method, and most of the times slower than the new method. I have the numbers if anyone needs them.

DirectFactorsOfGroupKN did not yet finish for most of these groups.

I still have some improvements (for example breaking the loop in the KN method) which I would like to implement, and I should get the whitespace changes into a separate commit if somebody helps me with that.

[markuspf]

Thanks for all that work! Out of curiosity: Did you also compare the results of the old DirectFactorsOfGroup to the new one (or verify the structure of the results otherwise)? If you have that testing code available, though having a long runtime, it'd be a good test to include somewhere. If you are keeping (detailed) runtimes, I'd be interested in them, too.

[hungaborhorvath]

@markuspf Yes, for the SmallGroups I compute the IdGroup of the results, sort it and compare the two lists (and they were identical for all groups tested).

For the bigger permutation groups, I compute the list of the sizes of the factors and compare them. These lists were identical, as well.

I have two files: testFunction.g for computing the direct factors, and then testCompare.g for actually comparing them. On my own laptop I usually check for SmallGroups of size 1-255, and only if that succeeds do I commit my changes and do the checks on the supercomputer.

If you tell me where I should look for format of the test files and some manual on how the test system works, then I can write some code that will check for several things. Then I could add that file to the pull request, as well.

[hungaborhorvath]

I also have the running times stored in files, just let me know how you want to get them.

[fingolfin]

Tests: I have no idea how you would want me to put the tests here.
I meant tests as we have them in *.tst files in e.g. tst/testinstall.

[ChrisJefferson]

We usually store files in tst/testinstall (if they take < 10 seconds) or tst/teststandard (if they take longer). If your .g files are quite long, turning them into .tst files might be annoying, and also not necessary.

My advice is to pop your files somewhere, with a guide as to how to run them, and we'll figure out how to put them into GAP. We need to get better at handling bigger tests, and making it easier ot add them, but we aren't there yet!

[olexandr-konovalov]

We have directory benchmarks - it may be worth to look there, and create another benchmark or maybe integrate into some existing one. Currently we run them weekly using Jenkins, but that's all configurable - e.g. could be monthly if they will became longer or on specific slaves etc.

[fingolfin]

You asked how to get rid of the spaces in the initial commit. You can do so using history rewriting. Beware, this will kill any uncommitted changes you have in your working tree, so make sure to first commit them, and/or stash them away using git stash.
Also, don't panic: All of this can be reverted. Just remember the id of the head commit of your branch (right now, this seems to be 2ae9c90) -- though even if you don't remember it, git can tell you later on using git reflog.

So here is what I just did to get rid of the spaces in my copy of your work:

• Find out the commit to change: That's a73a532.
• Use interactive rebase to edit that commit git rebase -i a73a532^ (note the trailing ^)
• This opens a list of commits since that commit, which you can edit -- there are instructions at the bottom. In this case, I just want to edit the first commit, so I change the word "pick" in the first line "e".
• save & close the list of commits, so that git rebase -i starts doing it magic. For me, that meant seeing this in my terminal:

~/Projekte/GAP/gap.github (git:pr-379)$ git rebase -i a73a532^
Stopped at a73a53238404e4e419502754fb71407404b0a894... Enhance DirectFactorsOfGroup for faster StructureDescription
You can amend the commit now, with

    git commit --amend

Once you are satisfied with your changes, run

    git rebase --continue

~/Projekte/GAP/gap.github (git:(v4.8.0-77-ga73a532))$

• Now I want to edit the commit, removing a few of the changes. There are multiple ways to go about that. If you are on Windows on Mac OS X, then SourceTree offers a very convenient GUI way to do this. So I entered this:

$ git revert -n head
$ git reset HEAD .
Unstaged changes after reset:
M   lib/grpnames.gd
M   lib/grpnames.gi
$ git add -p

• We can now interactively add chunks of changes. Git will show us part of the changes, and we can either stage that, or not, or skip. Basically, I pressed y or n a couple times, keeping hunks which undo whitespaces changes, and discarding those which undo your actual work. Sometimes I had to use s to split a hunk into smaller bits.
• Once this is done, I used git diff to verify that all remaining diffs are about undoing your actual work. Then I discarded them using git checkout lib. What remained where the staged changes, undoing whitespace changes. To be sure, I also verified that, using git diff --staged.
• Now I used git commit --amend to fold those whitespace changes back into the original commit. (this asks you to edit the commit message, but you can just keep the one you used before)
• Finally, I used git rebase --continue to let the interactive rebase finish.

Voila, the history is clean.

[markuspf]

We should collect this git toolbox that Max is explaining in issues on the
wiki as well as some links to git guides. I know, noone reads them, but I
started grepping through issues and mails to find Max' hints.

[hulpke]

Since I've been explicitly asked :-)

I did not follow the initial discussion as this is not really an issue for the groups I typically work with.

Looking at the code I'm puzzled at the inclusion of an operation ComplementsNormalSubgroup'. Why not useComplementclasses(or improve the functionality ofComplementclasses`)? I see little point in introducing private equivalents of existing functionality in the library.

[hungaborhorvath]

@hulpke ComplementNormalSubgroup returns a normal complement if exists, and fail otherwise. It does not return all normal complements, and you would not need them all, either, anyway. (At least for finding the direct factors you just want one.) Further, it does not find all subgroup complements, only the normal ones. Again, for direct factorization, you would only need the normal complements, so it seems a waste to compute all other complements.

I can imagine, that the ComplementClassesRepresentative algorithm could be rewritten to only compute the normal ones, but I do not understand it well enough to even try.

The current algorithm for ComplementNormalSubgroup is from the Kayal-Nezhmetdinov paper. I probably should include that into the manual.

Further, my initial testing was that ComplementClasses was slower for bigger groups. Now that I checked it again, the current version of ComplementNormalSubgroup is not always faster, and in fact, it sometimes is very slow. I found out the reason and will explain it in a separate post.

[hungaborhorvath]

So, I will need to tweak the ComplementNormalSubgroup algorithm, because it became slower in some situations than it was originally, when I did the very thorough testing.

The reason is that at some point (grpnames.gi:156-157) you need to take the intersection of the Center and a rightcoset of N, and find an element of a particular order in it. However, Intersection for a group and a coset takes a loooong time, so I decided to loop over the right coset. This was a huge increase for groups with relatively big centers (nilpotent groups with class 2) in the DirectFactorsOfGroup algorithm. For such groups it is important to try everything before computing NormalSubgroups, because there will be many normal subgroups....

However, for groups where N is big but the center is small, this algorithm becomes very slow. This did not seem to affect DirectFactorsOfGroup, because apparently you do not use ComplementNormalSubgroup in such situations. Nevertheless, I want to rewrite it so it will become more efficient in the situations that do not occur naturally calling DirectFactorsOfGroup.

I would like to ask some advice for this. My first idea would be to compute the Size of both the center and the right coset, and loop over the smaller one. However, Intersection should do exactly this for such a situation, but still it takes a much longer time, e.g.

gap> n := 9;; G := SymmetricGroup(n);; N := AlternatingGroup(n);;
gap> C := Center(G);
Group(())
gap> R := RightCoset(N, (1,2));
RightCoset(Alt( [ 1 .. 9 ] ),(1,2))
gap> Intersection(R, C); time;
[  ]
7080
gap> n := 9;; G := SymmetricGroup(n);; N := AlternatingGroup(n);;
gap> C := Center(G);
Group(())
gap> R := RightCoset(N, (1,2));
RightCoset(Alt( [ 1 .. 9 ] ),(1,2))
gap> Intersection(C, R); time;
[  ]
7480

So maybe Intersection should be the one rewritten? Then I could simply check the intersection for elements of particular order?

Another question: is there any faster way to find an element of a subgroup/right coset (their intersection) with a particular order?

Thank you.

[hungaborhorvath]

One more thing. In #552 it occurred that it might not be useful at all to have the direct factors as a Set, but only a list without duplicates and empty places. Is that the consensus? Because then I will rewrite the particular places, and the whole thing becomes slightly more efficient. Moreover, we could remove the function UnionIfCanEasilySortElements.

[markuspf]

The reason is that at some point (grpnames.gi:156-157) you need to take the intersection of the Center and a rightcoset of N, and find an element of a particular order in it. However, Intersection for a group and a coset takes a loooong time, so I decided to loop over the right coset. This was a huge increase for groups with relatively big centers (nilpotent groups with class 2) in the DirectFactorsOfGroup algorithm. For such groups it is important to try everything before computing NormalSubgroups, because there will be many normal subgroups....

Just to point out: Could you try benchmarking with @ChrisJefferson's "ferret" package loaded? https://github.com/gap-packages/ferret It features a vastly improved intersection method for permutation groups. (I assume you are actually using permutation representations here, which might be wrong and/or stupid). Cheers, Markus

[hungaborhorvath]

@markuspf On my home machine I ran the same commands as above with or wihout ferret 0.5.1 loaded, all of them ran for about 5 seconds.

[markuspf]

It looks like Chris' methods are not picked up.

Of course the intersection of the coset of the A_n and A_n in S_n is the pathological case for coset intersection, but one can test for this case separately (by detecting whether one is intersecting with a (coset of an) A_n, I suppose).

I suggest we debug this separately though as to not delay the merging of this PR too much longer. I will try to find out why the ferret methods are (apparently) not used.

I'll also ask Chris whether ferret even does Coset intersection yet (it should, and it should be orders of magnitude faster than the current GAP implementation).

[hulpke]

@hungaborhorvath
Sorry to drag this on: I think the name ComplementNormalSubgroup does not fit well -- something like NormalComplements would fit better with the existing names.

As there currently is no user-accessible implementation the existing code by definition is useful. However the intersection of group with coset, or the searching through a coset do not look efficient. A variant based on cohomology (as the existing code) is likely to perform better.

[hungaborhorvath]

@hulpke How about NormalSubgroupComplement then? I think Subgroup should be in the name, as well, but it should not have s in the end, considering that it only computes one complement.

BTW, if one complement is computed, then all the others can be computed by computing all possible homomorphisms from the normal subgroup to the center.

About efficiency, how about this: I will experiment a bit on how to find elements in the intersection of the center and a rightcoset with a particular order (it just occurred to me that because the center is abelian, it is easy to determine the elements of a particular order, so maybe it is faster to look through these whether they are in the right coset. Or I could use RationalClasses, as well.) And then after merging this, we could check if some othe method may perform faster?

[hungaborhorvath]

@hulpke I renamed ComplementNormalSubgroup to NormalComplement, as you requested.

Also rewritten parts of it to have it faster than it previously was. Applied some heuristic about whether to loop through elements of the Center or the right coset. After merging, a better Intersection method could be figured out. In any case, it should now run faster than ComplementClassesRepresentative.

I also added some more tests to check all lines of the new code.

@fingolfin @markuspf @hulpke Is there anything else I should do?

[markuspf]

I am for merging this now into master and if there's more to fix, we'll do it subsequently. I am sure @hungaborhorvath has a serious interest in having this work properly.

[markuspf]

Thank you @hungaborhorvath for sticking in there with this quite long process. I hope we'll get more high quality contributions from you in the future!

[hungaborhorvath]

Thank you very much for all your help. I learned a lot during these months.
If you notice anywhere anything fishy, please let me know, and I will (attempt to) fix it.

[olexandr-konovalov]

@hungaborhorvath thank you, glad to see this being merged! Now the next phase of testing starts ;-) There is a new diff in the manual example:

########> Diff in [ "./../../grp/classic.gd", 653, 663 ]
# Input is:
g:= Omega( 1, 4, 4 );  StructureDescription( g );
# Expected output:
Omega(+1,4,4)
"A5 x A5"
# But found:
Omega(+1,4,4)
"A5 : A5"
########

and I am not sure if the new output is better than the old one.

[hungaborhorvath]

@alex-konovalov No, this is a bug. I just dicovered it about 20 minutes ago (along with a completely different bug with something else). I will soon issue a PR to remedy this. Thank you.

[hungaborhorvath]

@alex-konovalov Should I put this into the changes from 4.8 to 4.9, as well? Or might this go into one of the smaller releases?

[olexandr-konovalov]

@hungaborhorvath this went into the master branch after stable-4.8 was started, so it will go into 4.9, so yes, please document it as well.

[hungaborhorvath]

@alex-konovalov I have written two sentences about this into the wiki page.