Add SemidirectDecompositions and enhance/change StructureDescription #763

hungaborhorvath · 2016-04-21T00:48:08Z

!!! WARNING !!! This changes the behaviour of StructureDescription
compared to the previous state, which was inconsistent with the manual.

SemidirectDecompositionsOfFiniteGroup is added. This computes all or some
semidirect decompositions of a group. An optional second argument can be
given as the normal subgroups for which complements should be sought.
Complements for normal subgroups are found by
ComplementClassesRepresentatives. Unfortunately, this does not work if
both N and G/N are nonsolvable.
Further, an optional argument "any" is recognized, when it should return
only one nontrivial semidirect decomposition if exists and fail otherwise.
This is mostly implemented by applying Schur-Zassenhaus by checking if
any nontrivial normal Hall subgroups exist. Another optional argument
"str" is recognized, when it does not necessarily compute the complement
to the normal subgroup if Schur-Zassenhaus applies. The reason is that
computing the complement might be expensive and for StructureDescription
only the isomorphism type of the complement is interesting.

SemidirectDecompositions is a new attribute, computing all semidirect
decompositions.

SemidirectFactorsOfGroup is removed completely. It computed all conjugacy
classes of all subgroups and therefore was rather inefficient. Further, it
did not compute all semidirect decompositions, but only the ones with
subgroups having the same size as the first subgroup having a normal
complement. This yielded inconsistent behaviour with the manual, the
smallest example for such a group was SmallGroup(504,7).

StructureDescription now works for infinite abelian groups, as well.
Further, for semidirect decompositions it calls
SemidirectDecompositionsOfFiniteGroup with "str" argument.

NOTE that if the group G has a nonsolvable normal subgroup N such that
G/N is nonsolvable, as well, then ComplementClassesRepresentatives errors
with "No method found", and thus so do SemidirectDecompositions and
StructureDescription. Previously computations for such groups did not error
with "No method found", but probably took a long time. #563 tries to
remedy this using the old and inefficient method, however whether it should
be merged or not is currently under debate. In the meantime, for such
groups we throw an error.

StructureDescription of groups having size at most 100 are recomputed, and the manual of StructureDescription is rewritten to match current behaviour.
Tests are added. All lines of SemidirectDecompositions are run. Not all
lines of StructureDescription are run, some examples for the missing lines
should be found later.
Tests are aligned with the new behaviour and not the old one.

Edit: The old documented behaviour of StructureDescription is still available.
If option nice is set, then all SemidirectDecompositions are computed
and one [N,H] will be chosen by the old preferences, namely abelian H,
abelian N with the most abelian invariants and the most direct factors,
and with H acting faithfully on N.

hungaborhorvath · 2016-04-21T01:13:05Z

Of course, tst/teststandard/bugfix.tst should fail, because now different semidirect products are found.

hungaborhorvath · 2016-04-21T01:28:52Z

For comparison:

gap> List(AllGroups(144), StructureDescription);; time; 
18560
gap> List(AllGroups(144), StructureDescriptionOld);; time;
62112
gap> List(AllPrimitiveGroups(DegreeAction,10), StructureDescription);; time;
448
gap> List(AllPrimitiveGroups(DegreeAction,10), StructureDescriptionOld);; time;
3928
gap> List(AllTransitiveGroups(DegreeAction,8), StructureDescription);; time;
4140
gap> List(AllTransitiveGroups(DegreeAction,8), StructureDescriptionOld);; time;
28292

Further, old StructureDescription for e.g. the following group would be hopeless:

gap> G := 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> StructureDescription(G); time; 
"C2 x (((((((C2 x ((C2 x D8) : C2)) : C2) : C2) : C2) : C2) : (C2 x PSL(3,2)))\
 x ((C2 x C2 x C2 x C2) : S5))"
22228

The smallest example of the behaviour change due to using ComplementClassesRepresentatives instead of the hard coded ConjugacyClassesSubgroups:

gap> G := Group([ (6,7,8,9,10), (8,9,10), (1,2)(6,7), (1,2,3,4,5)(6,7,8,9,10) ]);;
gap> StructureDescriptionOld(G); time; 
"(A5 x A5) : C2"
10756

For this group without #563 the new method errors out with "No method found".
With #563 I get this:

gap> G := Group([ (6,7,8,9,10), (8,9,10), (1,2)(6,7), (1,2,3,4,5)(6,7,8,9,10) ]);;
gap> StructureDescription(G); time; 
"A5 : S5"
11660

hungaborhorvath · 2016-05-05T01:47:03Z

Further, the new method significantly boosts the groups StructureDescription calculation for the groups mentioned in #160:

gap> StructureDescription(SmallGroup(256,56091)); time; 
"(C2 x ((C2 x ((C4 x C2) : C2)) : C2)) : C2"
16900
gap> StructureDescription(SmallGroup(512,10494180)); time;
"(C2 x ((C2 x ((C8 x C2) : C2)) : C2)) : C2"
45748
gap> StructureDescription(SmallGroup(64,266)); time;
"(C2 x ((C4 x C2) : C2)) : C2"
284
gap> StructureDescription(SmallGroup(128,2300)); time;
"((C4 x D8) : C2) : C2"
616
gap> StructureDescription(SmallGroup(128,2326)); time;
"(C2 x ((C2 x D8) : C2)) : C2"
1700
gap> StructureDescription(SmallGroup(128,2327)); time;
"(C2 x ((D8 : C2) : C2)) : C2"
1728

fingolfin

All in all, this looks like a very desirable change. I have only minor nitpicks, so basically I think this could be merged right away.

However, if you were willing to rebase this on current master, then codecov would get a chance to run on this, and we could see if the tests exercise all cases of your code.

Thank you very much!

fingolfin · 2016-11-03T13:59:51Z

lib/grpnames.g

-      "C2 x (C3 : C4)", "(C6 x C2) : C2", "C12 x C2", "C3 x D8", "C3 x Q8",
-      "S4", "C2 x A4", "C2 x C2 x S3", "C6 x C2 x C2" ], [ "C25", "C5 x C5" ],
+      "C2 x (C3 : C4)", "C3 : D8", "C12 x C2", "C3 x D8", "C3 x Q8", "S4",
+      "C2 x A4", "C2 x C2 x S3", "C6 x C2 x C2" ], [ "C25", "C5 x C5" ],
  [ "D26", "C26" ],
  [ "C27", "C9 x C3", "(C3 x C3) : C3", "C9 : C3", "C3 x C3 x C3" ],
  [ "C7 : C4", "C28", "D28", "C14 x C2" ], [ "C29" ],


Reading this diff is really nasty... which is not your fault, mind you! I think we should reformat NAMES_OF_SMALL_GROUPS so that there is exactly one string on each line.

But if I do that now, it would break this PR, so I am not going to do that on master for now. (If you feel bored and generous, feel free to do it as part of your PR, but then it only makes sense if you do it "twice": once in a commit before your change, were just the existing names are reformatted, and then of course after your changes, so that one can see the actual diffs in the relevant commit. Doing that of course is some work, so I am not asking you to perform that, nor am I going to hold your PR hostage over this! It's merely an idea)

I do not mind doing this, but I am not entirely sure what to do. I am not even sure how to rebase.

Is the rebase simply to go into my SemidirectDecompositions branch, and add the command

git rebase gap-system/master

(after of course fetching gap-system/master)?

And if this is done, then how should I add an extra commit where I reformat NAMES_OF_SMALL_GROUPS? Should I somehow go to an earlier commit with some git command, do the change, and commit?

Ok, I did
git rebase gap-system/master, then
git status gave me this:

On branch SemidirectDecompositions Your branch and 'origin/SemidirectDecompositions' have diverged, and have 303 and 4 different commits each, respectively. (use "git pull" to merge the remote branch into yours) nothing to commit, working directory clean

Then I did
git checkout becaeff915dbaef, and now I am stuck with the message

Note: checking out 'becaeff915dbaef'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by performing another checkout. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -b with the checkout command again. Example: git checkout -b new_branch_name HEAD is now at becaeff... Merge pull request #936 from markuspf/globalstate-fixes

I do not dare to do anything at this point until someone wiser suggests how to move forward instead of doing something irreversible....

Why did you do that checkout? Anyway, it makes little sense to me. What you want instead is to again checkout your local branch SemidirectDecompositions. That's the result of your rebase. It now diverse from origin/SemidirectDecompositions, i.e. what is on the server - that's normal and to be expected, as you performed a rebase. The larger number 303 of additional commits in the local branch SemidirectDecompositions stems from the fact that it now also includes all the work that happened on the master branch since april...
So, this work should now be pushed to your github clone of gap, which will then be reflected in this PR.

This should do it:

git checkout SemidirectDecompositions git push --force

Ok, I just did what you suggested and it seems to work.

I tried the checkout thing because I wanted to do the reformatting of NAMES_.... But I guess I should do something else to insert a commit between 0f1eb7d and becaeff?

One nice way to "insert" commits (or also edit, modify ec.) on a branch which is still work-in-progress (i.e. has not yet been merged into, say, master) is to use "interactive rebase". Here are some hints on that: https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history

But don't worry too much about this detail, it's in the category of "would be kinda nice, but in the end is unimportant".

Though I still recommend learning about interactive rebase, it's one of my favorite git features, I use it constantly for my branches

fingolfin · 2016-11-03T14:00:59Z

lib/grpnames.g

      "C22 x C2 x C2" ], [ "C89" ],
  [ "C5 x D18", "C9 x D10", "D90", "C90", "C3 x C3 x D10", "C15 x S3",
-      "C3 x D30", "C5 x ((C3 x C3) : C2)", "(C15 x C3) : C2", "C30 x C3" ],
+      "C3 x D30", "C5 x ((C3 x C3) : C2)", "(C3 x C3) : D10", "C30 x C3" ],
  [ "C91" ], [ "C23 : C4", "C92", "D92", "C46 x C2" ], [ "C31 : C3", "C93" ],
  [ "D94", "C94" ], [ "C95" ],, [ "C97" ],
  [ "D98", "C98", "C7 x D14", "(C7 x C7) : C2", "C14 x C7" ],


Many of these changes look like improvements to me; some perhaps are slightly worse -- but I think this is mostly subjective. In any case, I didn't notice anything that struck me as "bad", so I am fine with that part of the change.

fingolfin · 2016-11-03T14:02:33Z

lib/grpnames.gd

@@ -267,17 +268,56 @@ DeclareGlobalFunction( "DirectFactorsOfGroupKN", IsGroup );

 #############################################################################
 ##
-#A  SemidirectFactorsOfGroup( <G> ) . decomposition into a semidirect product


I was very mildly concerned about the removal (resp. renaming of SemidirectFactorsOfGroup). However, it was undocumented, and no package I am aware of uses it. So, in the end I have no objections to that.

I did a grep on the gap library for this undocumented function (no hits), and also asked @Stefan-Kohl. He advised to simply remove the old code.

We are in full agreement then 😀 (note the past tense in my comment).

fingolfin · 2016-11-03T14:03:25Z

lib/grpnames.gd

@@ -646,13 +686,14 @@ DeclareGlobalFunction( "LinearGroupParameters" );
 ##  <Mark>1.</Mark>
 ##  <Item>
 ##    Lookup in a precomputed list, if the order of <A>G</A> is not
-##    larger than 100 and not equal to 64.
+##    larger than 100 and not equal to 64 or 96.


Ahh, good catch :)

fingolfin · 2016-11-03T14:12:56Z

lib/grpnames.gi

+    fi;
+    return NHs;
+  fi;
+end);



I have not reviwed this code in detail, but on a cursory glance it looks sane, and I trust that you know what you are doing here... :-)

fingolfin · 2016-11-03T14:13:45Z

lib/grpnames.gi

@@ -1557,6 +1635,8 @@ InstallMethod( StructureDescription,
                                    " x ",""));
    fi;

+    if not IsFinite(G) then TryNextMethod(); fi;
+
    # special case alternating group


fingolfin · 2016-11-03T14:14:28Z

tst/teststandard/bugfix.tst

+#[ "6", "S3", "D12", "A4", "3xS3", "2xA4", "S4", "S4", "S3xS3", "(3^2):4", 
+#  "2xS4", "A5", "(S3xS3):2", "S5", "A6", "S6" ]
+#gap> StructureDescription(PSL(4,2));
+#"A8"



OK, so just remove that chunk (instead of commenting it out)

Ok, just removed in a new commit.

fingolfin · 2016-11-03T14:15:44Z

tst/testinstall/opers/StructureDescription.tst

+## gap> G := Group([ (6,7,8,9,10), (8,9,10), (1,2)(6,7), (1,2,3,4,5)(6,7,8,9,10) ]);;
+## gap> StructureDescription(G);
+## "A5 : S5"
+##


Is there a point to this commented out example? Is it too slow or what? Or why else is it commented out? I'd say, either remove this; or uncomment it; or else add a brief blurb that explains why it is commented out.

Yes, there is. Currently this example works. After merging this PR, this example will not work. The reason is that the new SemidirectDecompositions code uses ComplementClassesRepresentatives which does not work if both N and G/N is non-solvable. A proposed solution and a discussion about this arose in my PR #563. So this is a reminder to all of us that this example breaks after the PR is merged.

Added an explanation into the file.

fingolfin · 2016-11-03T14:16:01Z

tst/testinstall/opers/StructureDescription.tst

+
+##
+##gap> StructureDescription(AbelianGroup([0,2,3,4,5,6,7,8,9,10]):short);
+##"0x2520x60x6x2x2"


Hmm... see above

Ah, yes, this example would fail, because the value short does not currently work for infinite Abelian groups. For that to work, some essential change in the coding should be done, which I did not care to do at the time for mostly two reasons: this feature was not supported previously, anyway, and it would have taken me a longer time. Anyway, so this serves me as a reminder to do this at a later point. If it bothers you, I can remove it.

Added an explanation into the file.

fingolfin · 2016-11-03T14:17:32Z

tst/teststandard/bugfix.tst

 gap> StructureDescription(testG(8,3));
 "C3 x QD16"
 gap> StructureDescription(testG(8,4));
-"(C16 x C4) : C2"
+"((C16 x C2) : C2) : C2"


Ahh, too bad, the old ones looked "nicer" to me... but ignore me, it is probably impossible to making a "perfect" choice here (even if one assumed that all mathematicians had the same "taste", which I am sure we don't :-). Short of hard coding lots of cases, that is.

codecov-io · 2016-11-04T18:28:06Z

Current coverage is 49.44% (diff: 100%)

Merging #763 into master will increase coverage by 0.02%

@@             master       #763   diff @@
==========================================
  Files           424        424          
  Lines        222845     222948   +103   
  Methods        3430       3430          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         110120     110230   +110   
+ Misses       112725     112718     -7   
  Partials          0          0

Powered by Codecov. Last update 28b2660...0ee8cf5

fingolfin · 2016-11-04T22:27:57Z

Thanks for rebasing! The code coverage report suggests that all your new code (with the exception of one call to Error) is covered by tests. Excellent!

hungaborhorvath · 2016-11-05T09:33:33Z

@fingolfin I think I have managed to reformat NAMES_OF_SMALL_GROUPS in a commit preceding my first commit, and then in my first commit I reformatted again. I hope I did not mess anything up....

hungaborhorvath · 2016-11-05T10:00:53Z

Hm, tst/testinstall/timeout.tst fails. I have do idea what is in this test file, nor about what it tests. Further, it does not fail on my laptop, even after the reformatting....

Anybody has any suggestions?

ChrisJefferson · 2016-11-05T12:20:57Z

@hungaborhorvath : I strongly suspect it is not your fault -- this test occasionally fails for unpredicatable reasons. I've told travis to try re-running the test, to see if it fixes it.

hungaborhorvath · 2016-11-05T12:31:24Z

@ChrisJefferson You were right, apparently now travis succeeded.

fingolfin · 2016-11-09T13:03:59Z

Weird: When I run your new tests manually, I get a test failure:

~/Projekte/GAP/gap.github.playground (git:pr-763)$ ./bin/gap.sh -A -r
 ┌───────┐   GAP unknown of 2016-11-09 13:19:26 (CET)
 │  GAP  │   http://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin15.6.0-gcc-default64
 Libs used:  gmp, readline
 Loading the library and packages ...
 Components: trans 1.0, prim 3.0, small* 1.0, id* 1.0
 Packages:   GAPDoc 1.5.1
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> Test("tst/testinstall/opers/StructureDescription.tst");
########> Diff in tst/testinstall/opers/StructureDescription.tst:8
# Input is:
List(AllSmallGroups(16), G -> StructureDescription(G:recompute));
# Expected output:
[ "C16", "C4 x C4", "(C4 x C2) : C2", "C4 : C4", "C8 x C2", "C8 : C2", "D16",
  "QD16", "Q16", "C4 x C2 x C2", "C2 x D8", "C2 x Q8", "(C4 x C2) : C2",
  "C2 x C2 x C2 x C2" ]
# But found:
[ "C16", "C4 x C4", "(C2 x C2) : C4", "C4 : C4", "C8 x C2", "C8 : C2", "D16",
  "QD16", "Q16", "C4 x C2 x C2", "C2 x D8", "C2 x Q8", "Q8 : C2",
  "C2 x C2 x C2 x C2" ]
########
########> Diff in tst/testinstall/opers/StructureDescription.tst:54
# Input is:
StructureDescription(SmallGroup(32,8):recompute);
# Expected output:
"C2 . ((C4 x C2) : C2) = (C2 x C2) . (C4 x C2)"
# But found:
"C2 . ((C2 x C2) : C4) = (C2 x C2) . (C4 x C2)"
########
StructureDescription.tst
GAP4stones: 3
false

hungaborhorvath · 2016-11-09T20:22:14Z

@fingolfin After some playing around I figured out the reason: the elements of the list NormalSubgroups are ordered differently if you use -A -r. For example, the trivial subgroup is the first element, while running gap with no additional options the trivial subgroup is the last. The method for SemidirectDecompositions just runs through the list and finds the first normal subgroup having a complement (for efficiency reasons). I am not sure if anything can (or should) be done about this.

fingolfin · 2016-11-24T17:09:48Z

Well, I don't mind the diferent descriptions much. But this is an instability in the tests which can cause us some headaches in the future. It seems the test results differ whether packages are loaded or not, which is an issue, as @alex-konovalov will likely confirm ;-).

So, something should be done after all. As to whether (and what) can be done... One easy (but not very nice) option would to be modify the tests to skip these particular groups. Another might be to sort the normal subgroups. Now, a full sort is potentially slow, but it might suffice to sort them by size.

fingolfin · 2016-11-24T17:10:29Z

lib/grpnames.gi

-      return 0;
-    end;
+  if not IsBound(Ns) then
+    Ns := ShallowCopy(NormalSubgroups(G));


Regarding the tests differeing on which packages are present: Perhaps it would suffice to insert this here (have not yet had time to try it):

SortBy(Ns, Size);

@fingolfin @alex-konovalov I have tried it with -r -A -x 80 -m 100m -o 1g. It is not enough. For one, SmallGroup(16, 13) (and a lot of other groups of size <100) have different StructureDescription with and without packages. I could try to sort the NormalSubgroups completely according to some general way, but that would defeat efficiency. And I am not entirely sure how to do it, either.

Second, in StructureDescription.tst:25 the group will have A5 x A5 both with and without packages, but if no packages are present then maxsub.gi:545 will be invoked, and an info message "Alternating recognition needed" will be printed along with the output. Not sure if that can be nicely reconciled.

Finally, without packages the test runs in 8 seconds on my laptop, with packages it runs for 15 seconds. I am wondering if this is really how it should be....

I am not sure how to proceed at this point.

PS. Now that #563 is merged, I think I should rebase according to new master and uncomment the smallest example A5 : S5 in the tests. Would that be ok, or should I wait until the current problems are sorted?

@fingolfin @alex-konovalov Ok, rebased according to newest master and moved the slower tests to tst/teststandard/opers I also added a slow (about 1 min) test checking if grpnames.g contains the correct StructureDescription for the groups of size at most 100 (excluding 64 and 96).

No idea what to do with the discrepancies without packages, including the info message "Alternating recognition needed" and the differently ordered NormalSubgroups. Just ordering normal subgroups by size is not enough, a complete ordering defeats efficiency, removing the tests in question defeats the testing purpose.

Finally the following example (A5:S5) takes half time without packages (6 seconds without packages, 13 seconds with packages):

G := Group([ (6,7,8,9,10), (8,9,10), (1,2)(6,7), (1,2,3,4,5)(6,7,8,9,10) ]);; ConjugacyClassesSubgroups(G);;

@hungaborhorvath Thanks for the rebase.

As long as packages add new methods for things like NormalSubgroups, we will never be able to guarantee that these descriptions remain stable. And of course you are right, fully ordering the normal subgroups would be foolish in general.

So, perhaps we should simply stop any pretense that there is stability, and thus remove the relevant tests. Or only run them when the right packages are (not) loaded.

Another option would be to add another option, say stable or fullyinvariant or so to StructureDescription which instructs it to sort NormalSubgroups (assuming that is even enough). Then, compute the precomputed descriptions using that option, and use that option for the tests that verify the precomputed values. And indeed, do not run this test as part of the standard test suite. (I am not even sure it should be a regular test... Perhaps it would be better to have a function which (re)generates the file containing the precomputed names. Then, one can run that manually from time to time, if one is concerned about behavior changes).

Tests which fail at the slightest change of things are useless for us; esp. if mathemtically valid improvements cause these failures. Also, we never guarantee that structure descriptions are unique or never change, do we?

@fingolfin Hm, yes, maybe I should add an extra option stable or nicest or something. Actually, that way I could bring the old behaviour back for whoever still wants GAP to go through all possible descriptions and choose one by some rules (probably by the old rules just to keep consistent with old behaviour).

Another possibility that came to my mind is to have an option, say full, describing all possible descriptions (provided by the current code) into a set, and that would make tests completely stable. Using this we could rewrite our current tests such that if the result is an element of the full set or not. These probably would be long tests, maybe they should go into teststandard rather than testinstall?

For these changes I will need some time, though.

hungaborhorvath · 2016-11-27T15:12:36Z

@fingolfin I have stabilized the tests by adding back the old functionality under the option "nice". I have updated the documentation accordingly. The file grpnames.g now contains the nice descriptions for groups having size at most 100 (not 64 or 96).

Further, the "short" option is enhanced, it is faster than it was before, recognizes nonprimepower duplicates in the cyclic decomposition (C6 x C6 is now shortly 6^2) and also works for infinite abelian groups.

I believe the tests in tst/testinstall/opers/ check all new lines, and almost all lines of StructureDescription in grpnames.gi. The nonchecked lines are for simple groups from series C,2C,D,2D,3D,2F,2G, and for perfect groups which do not have any other decomposition.

At some later date I might write an option to determine all sensible descriptions, but not in the foreseeable future. Therefore I would rather have this merged if everybody is happy with it, and later I will do another PR for the new option.

hungaborhorvath · 2016-11-27T15:14:55Z

Ok, so now appveyor failed, and I have no idea what that is.... Can someone help me what I messed up here? Thank you.

ChrisJefferson · 2016-11-27T15:28:25Z

@hungaborhorvath : Just ignore that failure, it's a random computer hiccup.

olexandr-konovalov · 2016-11-27T15:39:48Z

@hungaborhorvath I've restarted the AppVeyor build, but from the output if was obvious that the cygwin64 build failed even before starting to compile GAP, so that was obviously not your fault. For OS-dependent PR it's better not to ignore failed AppVeyor builds though.

hungaborhorvath · 2016-11-27T16:42:43Z

@alex-konovalov This could be a short description into the 4.9.0 changelog:

SemidirectDecompositions is a new (undocumented) attribute, computing all decompositions of a group into semidirect products.

StructureDescription now works for infinite abelian groups and its speed is greatly enhanced, mostly by trying to find any kind of sensible description instead of choosing a relatively "nice" one from all possible descriptions. From now on, the option "nice" is recognized if one wants to use the old, (potentially much) slower behaviour. The manual page for StructureDescription is updated to reflect the changes.

hungaborhorvath · 2016-11-27T16:43:12Z

A side note: the undocumented new attribute and some new undocumented functions are fully documented in grpnames.gd. I simply did not want to document them into the manual and thereby completely cement their behaviour. I might do so at some point in the future, though.

olexandr-konovalov · 2016-11-29T12:04:22Z

@hungaborhorvath: My suggestion is to merge #965 first (I am testing it locally at the moment, so hopefully today) to ensure that tests are runnable, and then merge this PR of yours before it will require another rebase. Thanks for patience and polishing all tests!

hungaborhorvath · 2016-11-29T12:55:21Z

@alex-konovalov Thank you. After #965 is merged I believe I will need to slightly alter the test files because of the InfoWarning <-> InfoPerformance change.

!!! WARNING !!! This changes the behaviour of StructureDescription compared to the previous state, which was inconsistent with the manual. SemidirectDecompositionsOfFiniteGroup is added. This computes all or some semidirect decompositions of a group. An optional second argument can be given as the normal subgroups for which complements should be sought. Complements for normal subgroups are found by ComplementClassesRepresentatives. Unfortunately, this does not work if both N and G/N are nonsolvable. Further, an optional argument "any" is recognized, when it should return only one nontrivial semidirect decomposition if exists and fail otherwise. This is mostly implemented by applying Schur-Zassenhaus by checking if any nontrivial normal Hall subgroups exist. Another optional argument "str" is recognized, when it does not necessarily compute the complement to the normal subgroup if Schur-Zassenhaus applies. The reason is that computing the complement might be expensive and for StructureDescription only the isomorphism type of the complement is interesting. SemidirectDecompositions is a new attribute, computing all semidirect decompositions. SemidirectFactorsOfGroup is removed completely. It computed all conjugacy classes of all subgroups and therefore was rather inefficient. Further, it did not compute _all_ semidirect decompositions, but only the ones with subgroups having the same size as the first subgroup having a normal complement. This yielded inconsistent behaviour with the manual, the smallest example for such a group was SmallGroup(504,7). StructureDescription now works for infinite abelian groups, as well. Further, for semidirect decompositions it calls SemidirectDecompositionsOfFiniteGroup with "str" argument. NOTE that if the group G has a nonsolvable normal subgroup N such that G/N is nonsolvable, as well, then ComplementClassesRepresentatives errors with "No method found", and thus so do SemidirectDecompositions and StructureDescription. Previously computations for such groups did not error with "No method found", but probably took a long time. gap-system#563 tries to remedy this using the old and inefficient method, however whether it should be merged or not is currently under debate. In the meantime, for such groups we throw an error. StructureDescription of groups having size at most 100 are recomputed, and the manual of StructureDescription is rewritten to match current behaviour. Tests are added. All lines of SemidirectDecompositions are run. Not all lines of StructureDescription are run, some examples for the missing lines should be found later. Tests are aligned with the new behaviour, and not the old one.

This commit adds the old documented behaviour of StructureDescription. If option nice is set, then all SemidirectDecompositions are computed and one [N,H] will be chosen by the old preferences, namely abelian H, abelian N with the most abelian invariants and the most direct factors, and with H acting faithfully on N.

Further, grpnames.g contains the "nice" outputs of StructureDescription.

Now the option short works for infinite abelian groups. Further, it now recognizes nonprimepower duplicates, as well. E.g. the short StructureDescription for the group C6 x C6 is now 6^2.

hungaborhorvath · 2016-11-29T17:37:26Z

@alex-konovalov I rebased my PR to new master, otherwise tests would not have run because of the InforWarning->InfoPerformance change. In fact, I manually turn the level of these info levels to 0 and then back to where it was in the relevant lines in test files to keep these messages silent.

I manually tested the relevant files with and without packages, and squashed my change into one of my earlier commits, just to keep the history (sort of) clean.

olexandr-konovalov · 2016-11-29T22:16:41Z

@hungaborhorvath thanks. I am not sure that remembering the InfoLevel in the test is the right solution - this message occurs in many tests, and doing the same trick in each test seem not to be the right thing. Maybe START_TEST and STOP_TEST should be doing that, or maybe an undocumented InfoPerformance should be zero by default - but let's first hear from @hulpke.

FYI, there are only 7 messages in InfoPerformance in the GAP library.

olexandr-konovalov · 2016-12-01T13:09:22Z

@hungaborhorvath FYI, make testmanuals reveals diffs - they seem OK and I will replace examples with the new output:

########> Diff in [ "./../../lib/grpnames.gd", 821, 836 ]
# Input is:
List(AllSmallGroups(40),G->StructureDescription(G:short));
# Expected output:
[ "5:8", "40", "5:8", "5:Q8", "4xD10", "D40", "2x(5:4)", "5:D8",
 "20x2", "5xD8", "5xQ8", "2x(5:4)", "2^2xD10", "10x2^2" ]
# But found:
[ "5:8", "40", "5:8", "5:Q8", "4xD10", "D40", "2x(5:4)", "(10x2):2", 
 "20x2", "5xD8", "5xQ8", "2x(5:4)", "2^2xD10", "10x2^2" ]
########
########> Diff in [ "./../../lib/grpnames.gd", 821, 836 ]
# Input is:
List(AllTransitiveGroups(DegreeAction,6),
       G->StructureDescription(G:short));
# Expected output:
[ "6", "S3", "D12", "A4", "3xS3", "2xA4", "S4", "S4", "S3xS3", 
 "(3^2):4", "2xS4", "A5", "(3^2):D8", "S5", "A6", "S6" ]
# But found:
[ "6", "S3", "D12", "A4", "3xS3", "2xA4", "S4", "S4", "S3xS3", 
 "(3^2):4", "2xS4", "A5", "(S3xS3):2", "S5", "A6", "S6" ]
########

hungaborhorvath · 2016-12-01T20:20:25Z

@alex-konovalov Shoot... In the end I have forgotten to properly update the examples in the manual (BTW, all examples should be in the testinstall/opers/StructureDesctiption file). In fact, I also wanted to add an example about how to use the "nice" option. Further, I understand that now starting tests change InfoPeformance to 0, so I probably should remove those lines from my tests, as well.

I will put a new PR with these changes soon.

Further, correct the old examples from gap-system#763. Add some extra tests. Remove InfoPerformance from tst files as per 392ad82.

Further, correct the old examples from #763. Add some extra tests. Remove InfoPerformance from tst files as per 392ad82.

hungaborhorvath force-pushed the SemidirectDecompositions branch from 62534c1 to 8377194 Compare April 21, 2016 01:08

hungaborhorvath force-pushed the SemidirectDecompositions branch from 8377194 to 32e5719 Compare April 21, 2016 16:29

hungaborhorvath changed the title ~~Add SemidirectDecompositions and change StructureDescription~~ Add SemidirectDecompositions and enhance/change StructureDescription Apr 22, 2016

olexandr-konovalov added this to the GAP 4.9.0 milestone May 10, 2016

fingolfin approved these changes Nov 3, 2016

View reviewed changes

hungaborhorvath force-pushed the SemidirectDecompositions branch from 90d64ab to db5814b Compare November 4, 2016 18:49

hungaborhorvath force-pushed the SemidirectDecompositions branch from db5814b to e4e5cb5 Compare November 5, 2016 09:31

fingolfin reviewed Nov 24, 2016

View reviewed changes

hungaborhorvath mentioned this pull request Nov 25, 2016

Improved performance for group isomorphism/automorphisms #896

Merged

hungaborhorvath force-pushed the SemidirectDecompositions branch 5 times, most recently from 291e923 to 561d109 Compare November 27, 2016 12:39

hungaborhorvath mentioned this pull request Nov 28, 2016

Cleaning up grpauto.tst and other tests #964

Closed

4 tasks

hungaborhorvath added 11 commits November 29, 2016 18:02

Reformat NAMES_OF_SMALL_GROUPS

e7c9f8c

Replace StructureDescription outputs to new ones in bugfix.tst

6b1b2ba

Remove superfluous chunk

01b73b8

Add short explanation to commented tests

f797987

Check if both N and G/N are not solvable in SemidirectDecompositions

1f35f6b

Move some slower tests to teststandard

9c87c61

Add test for error message

c53e762

Make test files more resilient to StructureDescription

28e740b

Further, grpnames.g contains the "nice" outputs of StructureDescription.

Enhance short option in StructureDescription

0ee8cf5

Now the option short works for infinite abelian groups. Further, it now recognizes nonprimepower duplicates, as well. E.g. the short StructureDescription for the group C6 x C6 is now 6^2.

hungaborhorvath force-pushed the SemidirectDecompositions branch from ce20d70 to 0ee8cf5 Compare November 29, 2016 17:32

olexandr-konovalov merged commit acf0b2d into gap-system:master Dec 1, 2016

hungaborhorvath deleted the SemidirectDecompositions branch December 1, 2016 11:42

hungaborhorvath mentioned this pull request Dec 2, 2016

Add more examples into the manual for StructureDescription #969

Merged

olexandr-konovalov pushed a commit that referenced this pull request Dec 2, 2016

Add more examples into the manual for StructureDescription

46c3e87

Further, correct the old examples from #763. Add some extra tests. Remove InfoPerformance from tst files as per 392ad82.

This was referenced Dec 2, 2016

All kinds of errors in tst/testinstall/opers #970

Closed

Performance improvements and Corrections (in code used by group automorphisms) #968

Merged

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018

Add SemidirectDecompositions and enhance/change StructureDescription #763

Add SemidirectDecompositions and enhance/change StructureDescription #763

Conversation

hungaborhorvath commented Apr 21, 2016 • edited Loading

hungaborhorvath commented Apr 21, 2016

hungaborhorvath commented Apr 21, 2016 • edited Loading

hungaborhorvath commented May 5, 2016

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hungaborhorvath Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

hungaborhorvath Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hungaborhorvath Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 4, 2016 • edited Loading

Current coverage is 49.44% (diff: 100%)

fingolfin commented Nov 4, 2016

hungaborhorvath commented Nov 5, 2016

hungaborhorvath commented Nov 5, 2016

ChrisJefferson commented Nov 5, 2016

hungaborhorvath commented Nov 5, 2016

fingolfin commented Nov 9, 2016

hungaborhorvath commented Nov 9, 2016

fingolfin commented Nov 24, 2016

Choose a reason for hiding this comment

hungaborhorvath Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hungaborhorvath commented Nov 27, 2016

hungaborhorvath commented Nov 27, 2016

ChrisJefferson commented Nov 27, 2016

olexandr-konovalov commented Nov 27, 2016

hungaborhorvath commented Nov 27, 2016

hungaborhorvath commented Nov 27, 2016

olexandr-konovalov commented Nov 29, 2016 • edited Loading

hungaborhorvath commented Nov 29, 2016

hungaborhorvath commented Nov 29, 2016

olexandr-konovalov commented Nov 29, 2016

olexandr-konovalov commented Dec 1, 2016

hungaborhorvath commented Dec 1, 2016 • edited Loading

hungaborhorvath commented Apr 21, 2016 •

edited

Loading

hungaborhorvath commented Apr 21, 2016 •

edited

Loading

hungaborhorvath Nov 4, 2016 •

edited

Loading

hungaborhorvath Nov 4, 2016 •

edited

Loading

hungaborhorvath Nov 4, 2016 •

edited

Loading

codecov-io commented Nov 4, 2016 •

edited

Loading

hungaborhorvath Nov 25, 2016 •

edited

Loading

olexandr-konovalov commented Nov 29, 2016 •

edited

Loading

hungaborhorvath commented Dec 1, 2016 •

edited

Loading