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

Knowing the Center makes it impossible to compute NormalSubgroups #369

Closed
hungaborhorvath opened this issue Nov 22, 2015 · 29 comments
Closed
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements

Comments

@hungaborhorvath
Copy link
Contributor

For the following group, NormalSubgroups computes on my machine in about 50 seconds. However, if I compute the Center first (which is trivial for this particular group), then I only get

I Many (2097152) complements!

and do not ever get the normal subgroups. (I let it run for about half an hour, no change in the output.) I guess it is not an intended behaviour to not be able to compute something with having more information....

I came across this example when I was testing my new version of DirectFactorsOfGroup, where I check whether the center of the group is trivial or not, because if not, then one can pull back the direct factors of G/Center(G) without computing all normal subgroups. However, if this method fails, then I need to compute the normal subgroups and do a brute force search. In particular, it is useful to me to compute the center of the group.

Tested with the current master branch on debian linux. The result is the same with or without using the package CRISP.

Here is the relevant code:

gap> G := Group( (1,4)(2,3)(7,10)(8,9)(12,15)(13,14)(16,19)(17,18)(20,23)(21,22)(25,36)(26, 29)(28,31)(30,33)(32,35)(38,60)(39,48)(40,62)(41,64)(42,61)(43,63)(44, 53)(45,55)(47,51)(49,52)(50,54)(56,65)(59,66), (1,5,9)(2,10,6)(11,64,33)(12,62,31)(13,37,29)(14,35,16)(15,48,34)(17,55, 36)(18,60,25)(19,21,26)(20,53,27)(22,46,32)(23,63,30)(24,61,28)(39,42, 40)(41,43,44)(47,49,56)(50,65,58)(51,66,57)(52,59,54), (1,10)(5,6)(11,37)(12,62)(13,48)(15,60)(16,35)(17,61)(18,64)(19,26)(20, 55)(22,53)(23,63)(24,46)(25,32)(27,34)(28,33)(29,36)(38,50)(40,54)(41, 49)(42,52)(43,47)(45,51)(57,58)(65,66), (1,4,5,3,2)(6,8,9,10,7)(11,49,54, 50,53,25,39)(12,64,28,45,14,34,40)(13,17,59,57,62,35,32)(15,66,60,30,37, 16,41)(18,56,58,63,26,29,22)(19,42,20,65,55,31,46)(21,27,43,23,61,33, 38)(24,52,47,51,48,36,44), (17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65, 66), (4,6,7,5)(17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65,66) );;
gap> Center(G);
Group(())
gap> Size(NormalSubgroups(G)); time;

I Many (2097152) complements!

gap> G := Group( (1,4)(2,3)(7,10)(8,9)(12,15)(13,14)(16,19)(17,18)(20,23)(21,22)(25,36)(26, 29)(28,31)(30,33)(32,35)(38,60)(39,48)(40,62)(41,64)(42,61)(43,63)(44, 53)(45,55)(47,51)(49,52)(50,54)(56,65)(59,66), (1,5,9)(2,10,6)(11,64,33)(12,62,31)(13,37,29)(14,35,16)(15,48,34)(17,55, 36)(18,60,25)(19,21,26)(20,53,27)(22,46,32)(23,63,30)(24,61,28)(39,42, 40)(41,43,44)(47,49,56)(50,65,58)(51,66,57)(52,59,54), (1,10)(5,6)(11,37)(12,62)(13,48)(15,60)(16,35)(17,61)(18,64)(19,26)(20, 55)(22,53)(23,63)(24,46)(25,32)(27,34)(28,33)(29,36)(38,50)(40,54)(41, 49)(42,52)(43,47)(45,51)(57,58)(65,66), (1,4,5,3,2)(6,8,9,10,7)(11,49,54, 50,53,25,39)(12,64,28,45,14,34,40)(13,17,59,57,62,35,32)(15,66,60,30,37, 16,41)(18,56,58,63,26,29,22)(19,42,20,65,55,31,46)(21,27,43,23,61,33, 38)(24,52,47,51,48,36,44), (17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65, 66), (4,6,7,5)(17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65,66) );;
gap> Size(NormalSubgroups(G)); time;
35
48964

@hungaborhorvath
Copy link
Contributor Author

I did some checking about the known attributes of G after computing the center:

gap> KnownAttributesOfObject(G);
[ "Size", "OneImmutable", "LargestMovedPoint", "NrMovedPoints",
"MovedPoints", "GeneratorsOfMagmaWithInverses",
"MultiplicativeNeutralElement", "Centre", "OrbitsDomain",
"StabChainMutable", "StabChainOptions" ]

If I call Size and OrbitsDomain, then I have all of these attributes, except for the center, and GAP still computes the normal subgroups without a hitch:

gap> Size(G);
330301440
gap> OrbitsDomain(G);
[ [ 1, 4, 5, 10, 6, 9, 3, 7, 2, 8 ],
[ 11, 64, 37, 49, 41, 33, 18, 28, 61, 29, 16, 52, 56, 54, 43, 15, 42, 30,
38, 17, 60, 31, 24, 45, 26, 13, 36, 22, 19, 14, 35, 59, 47, 65, 58,
50, 40, 63, 44, 23, 12, 48, 66, 20, 21, 55, 25, 46, 51, 53, 34, 32,
57, 62, 39, 27 ] ]
gap> KnownAttributesOfObject(G);
[ "Size", "OneImmutable", "LargestMovedPoint", "NrMovedPoints",
"MovedPoints", "GeneratorsOfMagmaWithInverses",
"MultiplicativeNeutralElement", "OrbitsDomain", "StabChainMutable",
"StabChainOptions" ]
gap> Size(NormalSubgroups(G)); time;
35
48232

Therefore the Centre attribute must be the one that causes the problem. However, a simple

grep HasCent lib/*

only gave me lib/alglie.gi in the function LieCentre, which is about algebras, not groups.

@olexandr-konovalov
Copy link
Member

@hungaborhorvath Thanks. Could you please try to debug this using TraceMethods, ApplicableMethods etc? You may try this in the master branch here, which has some recent improvements by @ChrisJefferson.

@hungaborhorvath
Copy link
Contributor Author

If nobody will do it, then I will make an attempt. However, I have never used ApplicableMethod, in fact even ApplicableMethod(Size, G); gave me an error message. :-(

TraceMethod(NormalSubgroups, ChiefSeries); gives me the following if the Center is calculated:

gap> NormalSubgroups(G);;
#I NormalSubgroups: homomorphism principle perm groups at /home/ghorvath/work
/gap-system/gap/lib/grplatt.gi:1783
#I ChiefSeries: generic method for a group at /home/ghorvath/work/gap-system/
gap/lib/grpprmcs.gi:2651
#I Setter(ChiefSeries): system setter
#I ChiefSeries: system getter
#I ChiefSeries: system getter
#I Many (2097152) complements!

If the Center was not known originally, then I do not get the two ChiefSeries getter lines:

gap> NormalSubgroups(G);;
#I NormalSubgroups: homomorphism principle perm groups at /home/ghorvath/work
/gap-system/gap/lib/grplatt.gi:1783
#I ChiefSeries: generic method for a group at /home/ghorvath/work/gap-system/
gap/lib/grpprmcs.gi:2651
#I Setter(ChiefSeries): system setter
#I Setter(NormalSubgroups): system setter

Further, 'Many complements' error message (according to grep) can only be found in the function SubgroupsSolvableGroup, which cannot be traced by TraceMethod. It might be called from line 1934 of lib/grplatt.gi, but I do not know how to make sure. :-(

@olexandr-konovalov olexandr-konovalov added the kind: bug Issues describing general bugs, and PRs fixing them label Nov 23, 2015
@ChrisJefferson
Copy link
Contributor

@hungaborhorvath : Agree that the Chief Series looks very different.

We get there from NormalSubgroupsCalc (grplatt.gi:1783), the actual call to SubgroupsSolvableGroup is grplatt.gi:1952.

This branch of code is never run if we don't run Center first, I'm not entirely sure why, I'm hoping someone will pop up who is more expert in this.

It's possible (and would be annoying) that for some reason you just get a bad ChiefSeries, which is never promised to be unique. But I'm hoping someone can do better than that.

@olexandr-konovalov
Copy link
Member

I can reproduce this with GAP 4.6.5 and 4.5.7. Haven't tried other releases yet. Could someone try 4.4.12?

@ChrisJefferson
Copy link
Contributor

In fact, we can prove it's the ChiefSeries, because if call ChiefSeries(G) before Center(G), then things work fine, so knowledge of the Center seems to change how the ChiefSeries is calculated I think, and that leads to bad behaviour?

Unfortunately, this is well outside my area of expertise I'm afraid.

@fingolfin
Copy link
Member

@hungaborhorvath The correct syntax is ApplicableMethod(Size, [G]);

@fingolfin
Copy link
Member

@ChrisJefferson And ChiefSeries returns differing results because CompositionSeries does so. Moreover, StabChainMutable(G).orbit; returns differing results. But of course all of this is legal, only the factors in a chief series are fixed, not their orders. And indeed, we get the same compisition factors in both cases, i.e.

gap> SortedList(List([1..8], i-> IdGroup(cs[i]/cs[i+1])));
[ [ 2, 1 ], [ 2, 1 ], [ 8, 5 ], [ 8, 5 ], [ 8, 5 ], [ 16, 14 ], [ 60, 5 ], [ 168, 42 ] ]

Anyway, I thinkt the person with most expertise on this code is @hulpke so hopefully he knows more

@olexandr-konovalov
Copy link
Member

@hungaborhorvath and in addition, to make the output informative, G should not have size known already - otherwise you will get just the system getter instead of actual methods that may be applied.

@hulpke
Copy link
Contributor

hulpke commented Nov 23, 2015

On Nov 22, 2015, at 2:38 PM, hungaborhorvath notifications@github.com wrote:

For the following group, NormalSubgroups computes on my machine in about 50 seconds. However, if I compute the Center first (which is trivial for this particular group), then I only get

#I Many (2097152) complements!

and do not ever get the normal subgroups. (I let it run for about half an hour, no change in the output.) I guess it is not an intended behaviour to not be able to compute something with having more information….

I think this is an unavoidable side-effect: Calculating the center happens to use a chief series, which then is stored and this one particular chief series does not work well. This could happen in other situations.

What is happening is that the normal subgroups code — my ISSAC paper from ’98 - in some cases uses invariant subgroups as there seem to be many complements.

This is code which currently does not use the solvable radical, though this might be useful, if only for consistency.
I’ll have a look whether doing so will help.

Alexander

I came across this example when I was testing my new version of DirectFactorsOfGroup, where I check whether the center of the group is trivial or not, because if not, then one can pull back the direct factors of G/Center(G) without computing all normal subgroups. However, if this method fails, then I need to compute the normal subgroups and do a brute force search. In particular, it is useful to me to compute the center of the group.

Tested with the current master branch on debian linux. The result is the same with or without using the package CRISP.

Here is the relevant code:

gap> G := Group( (1,4)(2,3)(7,10)(8,9)(12,15)(13,14)(16,19)(17,18)(20,23)(21,22)(25,36)(26, 29)(28,31)(30,33)(32,35)(38,60)(39,48)(40,62)(41,64)(42,61)(43,63)(44, 53)(45,55)(47,51)(49,52)(50,54)(56,65)(59,66), (1,5,9)(2,10,6)(11,64,33)(12,62,31)(13,37,29)(14,35,16)(15,48,34)(17,55, 36)(18,60,25)(19,21,26)(20,53,27)(22,46,32)(23,63,30)(24,61,28)(39,42, 40)(41,43,44)(47,49,56)(50,65,58)(51,66,57)(52,59,54), (1,10)(5,6)(11,37)(12,62)(13,48)(15,60)(16,35)(17,61)(18,64)(19,26)(20, 55)(22,53)(23,63)(24,46)(25,32)(27,34)(28,33)(29,36)(38,50)(40,54)(41, 49)(42,52)(43,47)(45,51)(57,58)(65,66), (1,4,5,3,2)(6,8,9,10,7)(11,49,54, 50,53,25,39)(12,64,28,45,14,34,40)(13,17,59,57,62,35,32)(15,66,60,30,37, 16,41)(18,56,58,63,26,29,22)(19,42,20,65,55,31,46)(21,27,43,23,61,33, 38)(24,52,47,51,48,36,44), (17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65, 66), (4,6,7,5)(17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65,66) );;
gap> Center(G);
Group(())
gap> Size(NormalSubgroups(G)); time;
#I Many (2097152) complements!

gap> G := Group( (1,4)(2,3)(7,10)(8,9)(12,15)(13,14)(16,19)(17,18)(20,23)(21,22)(25,36)(26, 29)(28,31)(30,33)(32,35)(38,60)(39,48)(40,62)(41,64)(42,61)(43,63)(44, 53)(45,55)(47,51)(49,52)(50,54)(56,65)(59,66), (1,5,9)(2,10,6)(11,64,33)(12,62,31)(13,37,29)(14,35,16)(15,48,34)(17,55, 36)(18,60,25)(19,21,26)(20,53,27)(22,46,32)(23,63,30)(24,61,28)(39,42, 40)(41,43,44)(47,49,56)(50,65,58)(51,66,57)(52,59,54), (1,10)(5,6)(11,37)(12,62)(13,48)(15,60)(16,35)(17,61)(18,64)(19,26)(20, 55)(22,53)(23,63)(24,46)(25,32)(27,34)(28,33)(29,36)(38,50)(40,54)(41, 49)(42,52)(43,47)(45,51)(57,58)(65,66), (1,4,5,3,2)(6,8,9,10,7)(11,49,54, 50,53,25,39)(12,64,28,45,14,34,40)(13,17,59,57,62,35,32)(15,66,60,30,37, 16,41)(18,56,58,63,26,29,22)(19,42,20,65,55,31,46)(21,27,43,23,61,33, 38)(24,52,47,51,48,36,44), (17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65, 66), (4,6,7,5)(17,18)(27,34)(41,42)(49,52)(56,59)(61,64)(65,66) );;
gap> Size(NormalSubgroups(G)); time;
35
48964


Reply to this email directly or view it on GitHub.

@fingolfin
Copy link
Member

Ahh, of course -- I did not read the initial report carefully enough, and was under the impression that there was a wrong result. But now I see that the algorithm just takes a loooong time. That indeed makes sense.

Using the solvable radical method then indeed should resolve this (in particular, ensure this computation always succeds in a timely fashion, no matter whether you computed a center first or not ;).

@hungaborhorvath
Copy link
Contributor Author

I am sorry, but I do not understand. HasChiefSeries(G) returns false whether or not I calculate Center(G) first. In my second post I wrote what are the known attributes of G after computing the center, and ChiefSeries is not among them.

@fingolfin
Copy link
Member

No, but StabChainMutable is amongst them, and in the two cases, different stab chains have been computed, which then leads to other differences down the road.

@stevelinton stevelinton added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements and removed kind: bug Issues describing general bugs, and PRs fixing them labels Nov 23, 2015
@stevelinton
Copy link
Contributor

You can see the basic distinction in the following:

gap> $2)(49,52)(56,59)(61,64)(65,66) );;                                     
gap> ChiefSeries(G);
[ <permutation group of size 330301440 with 6 generators>, 
  <permutation group of size 165150720 with 11 generators>, 
  <permutation group of size 2752512 with 9 generators>, 
  <permutation group of size 172032 with 5 generators>, 
  <permutation group of size 1024 with 8 generators>, 
  <permutation group of size 128 with 7 generators>, 
  <permutation group of size 64 with 6 generators>, 
  <permutation group of size 8 with 3 generators>, Group(()) ]

gap>  G := Group( (1,4)(2,3)(7,10)(8,9)(12,15)(13,14)(16,19)(17,18)(20,23)(21,$
gap> Centre(G);; ChiefSeries(G);                                               
[     <permutation group of size 330301440 with 6 generators>, 
  <permutation group of size 1966080 with 12 generators>, 
  <permutation group of size 245760 with 13 generators>, 
  <permutation group of size 122880 with 10 generators>, 
  <permutation group of size 15360 with 7 generators>, Group([ (2,3,9,8), (2,
   4,9,7), (1,7,2,8,10,4,9,3), (1,5,10,6)(2,8,7)(3,4,9) ]), Group([ (1,4)(2,3)
  (7,10)(8,9), (2,4,3)(7,8,9), (3,5,4)(6,7,8), (1,10)(5,6), (2,9)(5,6), (1,10)
  (3,8), (4,7)(5,6) ]), Group([ (1,10)(5,6), (2,9)(5,6), (1,10)(3,8), (4,7)
  (5,6) ]), Group(()) ]
 gap> 

Neither Chief series is wrong, they just differ in the order of the factors, and it turns out one of them is better than the other for the algorithms that lists all the normal subgroups.

Obviously it would be good to improve the normal subgroups algorithm to automatically choose a good chief series, but I have no idea if this is feasible.

@hulpke
Copy link
Contributor

hulpke commented Nov 23, 2015

Having the series go through the solvable radical did not help (in fact it turns out in this example to always run in the bad case ….), but I think I have a solution:

The problem arises if in a subgroup there are tons of normal complements, but few of which are normal in the whole group.

In this case it is worth to look at the action on complements on the cohomology side (an affine linear action) to determine those which remain fixed under the whole group.

I have implemented this (the action is a bit tricky in the case of groups which are not solvable) and in the example it removes the problem (and gives a notable speedup). While teststandard.g runs OK, it has — as any such changes — the potential to mess up manual examples or package tests.

I’m happy to commit the change now, but I’m also happy to wait for after 4.8. Any opinions?

Best,

Alexander

@hungaborhorvath
Copy link
Contributor Author

I would vote for now. I would start it testing by testing my new DirectFactorsOfGroup algorithm.

@fingolfin
Copy link
Member

We don't normally vote on such things...

@hulpke Well, the "standard" way, IMHO, would be for you to open a pull request with your changes, so that people can test it (e.g. @hungaborhorvath ) and possible review it; also, the test suite is run on all PRs. Then we can decide whether to merge it.

@hulpke
Copy link
Contributor

hulpke commented Nov 24, 2015

@fingolfin I understand. However my setup (this is a relic of the switch to git...) is that I'm working off the main repository. I think to be able to do pull requests, I will need to work with a copied repository. Doing so is on my list for the winter break.

@fingolfin
Copy link
Member

@hulpke No, not at all. I do all my work in a single clone of the gap repository, just on multiple branches. :)

@hulpke
Copy link
Contributor

hulpke commented Nov 24, 2015

@fingolfin I looked at the documentation and it said that for pull requests I need to fork the main repository. I have not done this. Can I do a pull request in my situation?

@olexandr-konovalov
Copy link
Member

@hulpke OK - my understanding is that you have now some changes implemented in the clone of the main GAP repository, which you haven't committed yet, and now you would like to do a pull request, so you want to have them in fork. I think the following would work:

  • creating a fork via GitHub UI
  • using git remote command telling you clone that it's actually a fork.

Step-by-step guidance IMHO should be like that, assuming that you start in the directory gap which is a clone of git@github.com:gap-system/gap.git:

  1. git branch normal-subgroups to create branch
  2. git checkout normal-subgroups to switch to that branch
  3. commit your implementation in the newly created branch as usually (git add ..., git commit ...).
  4. DO NOT PUSH!!!
  5. instead, go to https://github.com/gap-system/gap
  6. click on the "Fork" button on the top-right corner and follow further instructions. When the process will complete, you will see the repository, but in the top left corner there will be something like hulpke/gap forked from gap-system/gap.
  7. You may of course now clone the fork with git clone git@github.com:hulpke/gap, and repeat steps 1-3 there, but we don't want to spend time on that - we will use git remote instead - open the terminal, go to the directory gap of your clone, and do the next step
  8. git remote -v to inspect it.

Now you will see that it points to the main repository

$ git remote -v
origin  git@github.com:gap-system/gap.git (fetch)
origin  git@github.com:gap-system/gap.git (push)

To change this, do:

  1. git remote remove origin to erase this info.
  2. git remote add origin git@github.com:hulpke/gap to point it to the new one.
  3. git remote -v to inspect the changes. Please make sure that it points to the URL with hulpke instead of gap-system before going to the next step.
  4. git push --set-upstream origin normal-subgroups to push changes to a branch.

After that, others will be able to see your changes even before you will submit a pull request. I suggest to do this first (you will need to do these steps only once!), and then think about PR after that.

@hulpke
Copy link
Contributor

hulpke commented Nov 25, 2015

@alex-konovalov
Thank you very much. I have now created a fork
hulpke/gap

in my part of git. My setup now (on office and home machine) is:

[Abulafia:~/gap4] ahulpke% git remote -v
origin https://github.com/hulpke/gap.git (fetch)
origin https://github.com/hulpke/gap.git (push)
upstream https://github.com/gap-system/gap.git (fetch)
upstream https://github.com/gap-system/gap.git (push)

In origin I created a branch
ah
that contains the improvement. What do I do now?

Further questions (Sorry -- I have not used these features before and did not find this for sure in the documentation):

  • How do I keep my repository updated with the changes from gap-system/gap?
  • Do I need to move any changes I do that should go back to the main repository through pull requests?
  • (Probably stupid, but enormously irritating) On my office machine (strangely enough not on the home machine) now any push asks me for username and password. Following the documentation I have set
    git config --global credential.helper osxkeychain
    but that so far did not seem to help. Any idea?

Thank you!

Alexander

@olexandr-konovalov
Copy link
Member

@hulpke Great - welcome to the forking club :) Trying to answer your questions,

Requesting password for authentication

I think you've used instructions from elsewhere and configured https access instead of ssh which you have used before. Do this:

git remote remove origin
git remote remove upstream
git remote add origin git@github.com:hulpke/gap.git
git remote add upstream git@github.com:gap-system/gap.git

You may call git remote -v before and after to inspect remotes.

Creating a branch and its lifecycle

It is suggested to pick up more informative name than ah. Note that pull request from such named branch will be displayed as hulpke wants to merge N commits into gap-system:master from hulpke:ah, so initials in the branch name are redundant (that was different when we have used Mercurial, I remember). A good practice is to use the name which reflects the nature of the proposed change, e.g. normal-subgroups. The lifecycle of a branch is shorter than it was in our Mercurual development model when you had one branch for each of the two release branches, and merged them periodically, and then continued to add new changes in these branches. Here, in Git, after pull request (PR for short) will be merged, the branch may be safely thrown away as the change is already in the main repository. It's common to have several branches at a time, each for different things, which may be reviewed and merged independently at their own speed.

Creating Pull Request

See this help page. Often this is even more simpler - if you've just pushed your branch to your fork, then when you will open the repo on GitHub, it will expect you creating a PR and will suggest this, and it will by default compare your branch with master branch of the main repo.

How do I keep my repository updated with the changes from gap-system/gap?

Please see Syncing a fork

Do I need to move any changes I do that should go back to the main repository through pull requests?

No - for small obviously correct changes, if you have write access to the main repository, it's fine to do them there. Pull requests are useful for things that we want:

  • to be tested with Travis CI before merging them, to reduce the risk of incidentally breaking things in the release branches
  • to be reviewed
  • to be known better by others
  • to be included into the milestone for the next release and the overview of changes.

@olexandr-konovalov
Copy link
Member

This should be fixed by #376 and may be closed as soon as that PR will be merged and pass the tests.

@hungaborhorvath
Copy link
Contributor Author

There are still situations when choosing a bad chiefseries makes the computations ridiculously longer.

gap> tpg := ReadAsFunction("Df/testpermgroups.g")();;
gap> G := tpg[236];
Group([ (1,2)(3,4)(5,6)(7,8)(9,10)(11,12)(13,14)(15,16)(17,18)(19,20), (1,3)
(5,7)(9,11)(13,15)(17,19), (1,4)(6,9)(11,14)(16,19) ])
gap> Center(G);
Group(())
gap> NormalSubgroups(G);; time;
14816

gap> tpg := ReadAsFunction("Df/testpermgroups.g")();;
gap> G := tpg[236];
Group([ (1,2)(3,4)(5,6)(7,8)(9,10)(11,12)(13,14)(15,16)(17,18)(19,20), (1,3)
(5,7)(9,11)(13,15)(17,19), (1,4)(6,9)(11,14)(16,19) ])
gap> NormalSubgroups(G);; time;
860

Not sure if anything could be done about this, just reporting. Have some more examples like that if needed.

@bh11
Copy link
Contributor

bh11 commented Dec 10, 2015

@hungaborhorvath
In the case of tpg[236], both variants are about equally fast when switching to a GAP version prior to pull request #376.

@hungaborhorvath
Copy link
Contributor Author

Using the following test files, it is possible to compare the two methods, if anybody wants that:

http://riemann.math.unideb.hu/~ghorvath/testDf.tar.bz2

I checked for groups of size up to 255. This test showed that the new method was at least 200ms and at least 10% faster for 21 groups, the old method was faster for 32 groups. However, one should probably check fo big permutation groups. There are some in the attached file in testpermgroups.g.

@hungaborhorvath
Copy link
Contributor Author

Ok, I did some tests on big permutation groups. The new method ran out of memory sooner than the old one (computed on 4667, while the old method computed on 4670). Further, the new method was faster (by at least 200ms and by at least 10%) for 79 groups, while the old method was faster for 92 groups. The maximal difference was 12sec.

@hungaborhorvath
Copy link
Contributor Author

If anyone is interested, I run the old and the new NormalSubgroups method for SmallGroup order different from 512, 768, 1024, 1280, 1536, 1792 (up to order 2015), and on about 8000 big permutation groups. The outputs were always the same (for the permutation groups the sizes of the normal subgroups were the same), and the computing times were about the same, as well.

I considered two computing times different if the difference was at least 200ms and at least 10%. For SmallGroups the old method was faster 2104 many times, of which 154 many times by the amount 1-10 seconds. The new method was faster 2311 many times, of which 19 times by the amount 1-10 seconds and once by 17 seconds.

For the big permutation groups, the old method was faster 391 many times, the new method was faster 297 many times. As for the differences, the old method was faster by 100-400 ms twice, one of the group was this:

gap> H := tpg[6883];
<permutation group with 3 generators>
gap> GeneratorsOfGroup(H); 
[ (6,7)(8,9)(11,12)(13,14)(15,16)(18,19)(21,22)(23,24)(26,27)(29,30)(31,
    32)(34,35)(37,38)(39,40)(41,42), (4,5)(6,8)(9,12)(18,21)(22,24)(25,28)(31,
    34)(35,38)(45,46)(49,50)(52,53)(56,57)(59,60)(63,64), 
  (1,11)(2,18)(3,26)(4,34)(5,41)(7,44)(9,46)(12,47)(14,48)(16,49)(19,51)(22,
    53)(24,54)(27,55)(30,56)(32,58)(35,60)(38,61)(40,62)(42,63) ]

The new method was faster 4 times by at least 100 seconds, and once by 20 minutes for the following group:

gap> G := tpg[4530];
<permutation group with 3 generators>
gap> GeneratorsOfGroup(G); 
[ (4,5)(7,8)(11,12)(14,15)(17,18)(20,21)(26,27)(28,29)(31,32)(33,34)(37,
    38)(40,41)(43,44)(46,47)(49,50)(55,56)(58,59)(60,61)(66,67)(70,71)(79,
    80)(84,85)(88,89), (5,6)(9,10)(13,14)(17,18)(22,23)(25,26)(28,29)(36,
    37)(40,41)(44,45)(50,51)(52,53)(56,57)(61,62)(63,64)(67,68)(71,72)(73,74),
  (1,7)(2,17)(3,37)(5,42)(8,45)(10,46)(12,48)(13,49)(15,52)(18,54)(21,57)(23,
    58)(25,60)(27,63)(29,65)(32,68)(34,69)(36,70)(38,73)(41,75)(44,76)(47,
    77)(50,78)(53,79)(56,81)(59,82)(61,83)(64,84)(67,86)(71,87)(74,88)(80,
    90)(85,91)(89,92) ]

I am guessing that the bigger differences come from starting from a different chiefseries, but I am not sure. If anyone is interested, I can put up these test files to somewhere for download.

Otherwise, if there are no more comments, I think this issue could be closed.

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
Projects
None yet
Development

No branches or pull requests

7 participants