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

Add ComplementClassesRepresentatives for all groups #563

Merged

Conversation

hungaborhorvath
Copy link
Contributor

This is the third installment in the series of enhancing
StructureDescription. (The first being DirectFactorsOfGroup, the second is
NormalHallSubgroups.)

This commit adds a method to ComplementClassesRepresentatives by
computing all conjugacy classes of subgroups, and then checking them
one-by-one for a suitable complement. It has a very low rank, and (just in
case) checks first whether some faster method cannot be used. Further, it
warns the user that all subgroups are computed, because this method can be
really inefficient.

In the fourth installment, SemidirectDecompositions will be written to use
ComplementClassesRepresentatives to determine whether or not a group can
be factored as a semidirect product.

Moreover, added a new KeyDependentOperation named
ComplementClassesRepresentativeKD, which calls
ComplementClassesRepresentative, but stores the result as a usual
KeyDependentOperation does. I believe this can be useful, and it should go
to the manual, but I would need help with that.

Added test file, which at least runs through the new code, and parts of
the old code, as well.

@hungaborhorvath
Copy link
Contributor Author

@hulpke I would appreciate any feedback from you on this. Thank you.

H := Representative(Hc);
if not IsTrivial(H) and G<>H and IsTrivial(Intersection(N, H)) and
(( CanComputeSize(G) and CanComputeSize(N) and CanComputeSize(H) and
Size(G) = Size(N)*Size(H) ) or G = ClosureGroup(N, H) ) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably test Size(G)=Size(N)*Size(H) first, because in most cases this will cut down the number of groups to intersect enormously.

@bh11
Copy link
Contributor

bh11 commented Jan 31, 2016

Do you really need the KD variant for your code? Otherwise, I'd just leave it out. Using normal subgroups as keys may be highly inefficient for f.p. gropus and matrix groups (esp. because of the sortedness).

@hungaborhorvath
Copy link
Contributor Author

@bh11 I believe the user should have the option to do this.

In the SemidirectDecompositions code, I will have an extra argument, where the user can say "any", meaning it will stop after computing one semidirect decomposition. In general, on would only need this, and would not want to compute all semidirect decompositions. However, after computing one decomposition (in a possibly long time), it is annoying to recompute it just because they forgot to store it in a variable. (Of course, they would still need to use the KeyDependent version, so one can argue that if the user remembers to use that, then they could remember storing the value, as well.)

I already discussed this with @hulpke last year at some point, and he was also against storing the complements for every normal subgroup in general. Of course, if many are against the idea, I can just remove the code, and be done with it. In any case, it is not essential to any of my future codes.

@hungaborhorvath
Copy link
Contributor Author

@bh11 I have moved the size test first and removed the two trivial case tests.

@hulpke
Copy link
Contributor

hulpke commented Jan 31, 2016

Since there was an explicit request for feedback from me:

I'm unsure on the wisdom of implementing embarrassingly elementary fallback routines (with the argument that nothing better is available) for some operations -- If anyone knowledgeable looks at the code they might leave with a bad impression of the overall system.

Concerning complements, if the factor group is solvable (if both normal subgroup and factor group are non solvable one can reduce - by Schreier's (proven) conjecture - to the case of centralizers) there is a function COSolvableFactor(G,N,M) (M will be the trivial subgroup or a normal subgroup modulo which complements are to be computed) in grppccom.gi which follows the algorithm from my ISSAC '13 paper (where it is used for a very specific case of subgroup lattice computation). It likely is still suboptimal (and that was the reason why I did not link it yet into `ComplementClassesRepresentatives), but it will run circles around computing the whole lattice.

I agree with @bh11 on questioning the need for the extra KD operation.

@hungaborhorvath
Copy link
Contributor Author

@hulpke Thank you for your feedback. I agree that this fallback method is ridiculous, however (as you mention) there is currently nothing else. That is why it gives an info warning. It should be considered which is the worse of the two evils: having slow and trivial algorithms or to error out with "No method found". This is not my place to decide, but I am happy to rescind this PR if the consensus is to do that.

About Schreier's conjecture: I would be hesitant to base a default method on this theorem, because the only known proof uses the classification. Maybe a method that could be called with an option? Nevertheless, currently this method is not implemented, either, and I am not sure I would be able to implement it. (I see how it would work for a simple normal subgroup, but not when needed to work through a chief series.)

I do not entirely understand your comment on COSolvableFactor. Currently this is the default method if N is not solvable but G/N is solvable in grppccom:1194.

In any case, the reason I would like something to work for all groups is to being able to check with the future code of StructureDescription whether the group can be written as a semidirect product, and would like to use ComplementClassesRepresentatives for it. However, this currently does not work if both N and G/N are not solvable. The smallest such group is A_5 semidirect S_5 with 7200 elements. We should decide then whether for such groups StructureDescription should exit in some graceful way, or should still try to give some understandable answer (this PR is a step towards this), or something else. I am not sure what the general user preference would be, but I would be interested in your (and all of the GAP developer's) preference.

I have removed the KeyDependent version of ComplementClassesRepresentative.

@markuspf
Copy link
Member

markuspf commented Feb 1, 2016

My personal opinion is that having something that works, i.e. a slow and ridiculous default method, always beats having nothing. This is of course only true as long as it gives correct answers, and maybe displays a warning saying that the computation might take a little longer (say, beyond the end of the universe).

As to whether one relies on proofs that use the classification is a rather tickly question. I think as long as it is documented, I don't see a problem with it.
Even to the contrary: Lets use the assumption that assume the classification is correct in places and document it. Lets also do computations that use other ways. If we find inconsistencies, all the better, and if we don't, it's also good.

@bh11
Copy link
Contributor

bh11 commented Feb 1, 2016

I'd actually remove this entire last resort method. The error message might even be misleading once there are methods for insoluble N which, for some reason, don't get selected. Instead, such an error should trigger a "No method found" error.

@hungaborhorvath If you need a method for insoluble N, then you could install your method for groups having ConjugacyClassesSubgroups, and make sure that your code computes that attribute before calling ComplementClassesRepresentative. If you need to find complements for a lot of normal subgroups, computing ccls. of all subgroups probably isn't even the worst solution.

Btw., if one needed complements for a single normal subgroup, one might have a look at LatticeByCyclicExtensions which allows to compute subgroups dividing the required size only, but this only makes sense if the group is not too big and has a "good" representation (i.e., perm group).

But I guess that your StructureDescription code works via NiceMonomorphisms anyway if the group in question is handled by them.

@hulpke
Copy link
Contributor

hulpke commented Feb 1, 2016

@hungaborhorvath

I do not entirely understand your comment on COSolvableFactor. Currently this is the default method if > N is not solvable but G/N is solvable in grppccom:1194.

Oops. Then I misremember what I did -- I remember that I was debating with myself about the wisdom of dong so.

So the only case remaining is the one of non solvable normal subgroup and non solvable factor group. As I wrote I think the proper way of doing this (which actually won't be that horribly much effort) is by computing centralizers. This is very similar how the NormalSubgroups routine works -- one can use Bray's method of involution centralizers.

Concerning the use of Schreiber's conjecture: The proof uses the classification only for the all simple groups'' part. If we weaken it toall known simple groups'' it is harmless.

By the time you have normal subgroups you have used the classification already in a stronger sense for composition series. Furthermore classes of elements, (normal) subgroups and in the end (via presentations) complement classes all use the classification implicitly.

@hungaborhorvath
Copy link
Contributor Author

@bh11 I like your idea, just pushed a change making this method dependent on previously computed ConjugacyClassesSubgroups. Further, I added back the old "error out" method, but updated the error message with more current information. What do you think @hulpke ?

Modified the tst file, added the smallest example for a semidirect product of two nonsolvable groups (A5:S5). Added a small example, as well.

@hulpke It would be great if you could write a fast method for nonsolvable N and nonsolvable G/N if it is not too much of an effort.

About the classification: I think it would be really important to document which algorithms use currently the classification and in what sense. I will raise a new issue about this.

@fingolfin
Copy link
Member

As explained on #571, I don't think it makes sense to worry about Schreier here -- Schreier is certainly true for all groups GAP can currently deal with.

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Feb 10, 2016
@markuspf markuspf added this to the GAP 4.9.0 milestone Mar 7, 2016
hungaborhorvath added a commit to hungaborhorvath/gap that referenced this pull request Apr 21, 2016
!!! 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.
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.
hungaborhorvath added a commit to hungaborhorvath/gap that referenced this pull request Apr 21, 2016
!!! 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.
hungaborhorvath added a commit to hungaborhorvath/gap that referenced this pull request Apr 21, 2016
!!! 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.
@markuspf markuspf added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label May 5, 2016
hungaborhorvath added a commit to hungaborhorvath/gap that referenced this pull request Nov 4, 2016
!!! 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.
TryNextMethod();
fi;
Error("Cannot compute complement classes for nonsolvable normal subgroups");
Error("Cannot compute complements if both N and G/N are nonsolvable");
end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the discussion, @bh11 suggested ommitting the error, instead letting the user see a "no method found" error. I tend to agree with that, for the reasons he gives.
However, there is some merit for this error, too, in that it may be helpful for a user to understand why ComplementClassesRepresentatives works for one group but not another.
Then again, the same would probably apply for lots of other operations in GAP, so I wonder whether this particular one is really so special to deserve special treatment?

I guess the alternative then is to always invoke TryNextMethod()? Or perhaps use Redispatch? (that wouldn't work for the factor group though, would it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally do not mind one way or another. @hulpke put the original error message in the code, I just updated it to better reflect the current situation. Anyway, just let me know what to do with it.

fi;
return C;
end);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Of course it is a "naive" method and could have horrible performance, but I tend to agree with @markuspf that this is still better than an error. Regarding the concern that this "suboptimal" algorithm could tarnish GAP's reputation, I say two things: First off, let's then just add a brief comment here that explains this: "This is a very naiv algorithm; we only provide it as a last resort, ideally it is never invoked". Secondly: if we are concerned about that, there is a lot of code in GAP we should rip out. There are far too many times when trying to do a seemingly simple operation, I cought GAP trying to enumerate a huge or even infinite set. Even things like trying to compute the normalizer of the trivial group can run into an infinite computation in GAP -- for perfectly fine reasons, of course, but still, this seems far worse to me than the above method... And the above method in turn seems much better to me than saying "oops, don't how to do that" when people ask GAP to do this for tivial examples (like G = A5xA5)

gap> G := SymmetricGroup(6);; N := AlternatingGroup(6);;
gap> Set(ComplementClassesRepresentatives(G, N), H -> H^G)=Set([ Group([ (1,2) ])^G, Group([ (1,2)(3,4)(5,6) ])^G ]);
true
gap> STOP_TEST("ComplementClassesRepresentatives.tst", 10000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are always good.

As with your other PRs, I'd appreciate if you could rebase this, so that the code coverage checks run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did it, see if tests run fine.

This is the third installment in the series of enhancing
StructureDescription. (The first being DirectFactorsOfGroup, the second is
NormalHallSubgroups.)

This commit adds a method to ComplementClassesRepresentatives by
computing all conjugacy classes of subgroups, and then checking them
one-by-one for a suitable complement. It has a very low rank, and (just in
case) checks first whether some faster method cannot be used. Further, it
warns the user that all subgroups are computed, because this method can be
really inefficient.

In the fourth installment, SemidirectDecompositions will be written to use
ComplementClassesRepresentatives to determine whether or not a group can
be factored as a semidirect product.

Moreover, added a new KeyDependentOperation named
ComplementClassesRepresentativeKD, which calls
ComplementClassesRepresentative, but stores the result as a usual
KeyDependentOperation does. I believe this can be useful, and it should go
to the manual, but I would need help with that.

Added test file, which at least runs through the new code, and parts of
the old code, as well.
Instead of a general fallback method, the new method only runs for
groups having ConnjugacyClassesSubgroups computed already. The old
fallback method (erroring on nonsolvable N and G/N) is returned,
but in a way that is more consistent with the current existing methods.

One more test added, and the big permutation group test is replaced
by the smallest possible direct indecomposable semidirect product of
two nonsolvable groups (A_5 and its automorphism group S_5).
@fingolfin
Copy link
Member

@markuspf You added the "DO NOT MERGE!" label; I am not quite sure why this PR got it, though. Perhaps this was based on an older version of it? The current one seems quite fine to me.

@markuspf
Copy link
Member

markuspf commented Nov 4, 2016

I am pretty sure this was because of earlier concerns by @hulpke, but I do not remember. I am happy to take it off though.

@hungaborhorvath
Copy link
Contributor Author

hungaborhorvath commented Nov 4, 2016

As ComplementClassesRepresentatives is basically @hulpke's function I would appreciate if he also gave his ok on this PR, if he does not mind taking another look.

@codecov-io
Copy link

codecov-io commented Nov 4, 2016

Current coverage is 49.30% (diff: 100%)

Merging #563 into master will increase coverage by 0.47%

@@             master       #563   diff @@
==========================================
  Files           424        424           
  Lines        222089     222106     +17   
  Methods        3426       3426           
  Messages          0          0           
  Branches          0          0           
==========================================
+ Hits         108441     109508   +1067   
+ Misses       113648     112598   -1050   
  Partials          0          0           

Powered by Codecov. Last update becaeff...f9732fa

hungaborhorvath added a commit to hungaborhorvath/gap that referenced this pull request Nov 5, 2016
!!! 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.
TryNextMethod();
fi;
Error("Cannot compute complement classes for nonsolvable normal subgroups");
Error("Cannot compute complements if both N and G/N are nonsolvable");
end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://codecov.io/gh/gap-system/gap/pull/563/compare this method is completely untested. Perhaps we can come up with a test case? E.g. creating a permutation group plus subgroup from generators, which initially does not know it is solvable... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added tests for the errors and for the G=N case. However, this TryNextMethod should never trigger, because this whole method only triggers if the main method exits with TryNextMethod, that is if both N and G/N are nonsolvable.

elif G = N then
C := [ TrivialSubgroup(G) ];
elif not IsNormal(G, N) then
Error("N must be normal in G");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error, and the case G=N above, are not currently covered by the tests, see https://codecov.io/gh/gap-system/gap/pull/563/compare

@fingolfin fingolfin removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Nov 5, 2016
@fingolfin
Copy link
Member

I agree that @hulpke ideally should have another look (he is travelling right now, though)

@fingolfin
Copy link
Member

@hulpke Would you mind having another (quick) look at this? Is it acceptable at all to you, or still fundamental objections?

@hulpke
Copy link
Contributor

hulpke commented Nov 24, 2016

@fingolfin: I'm OK with adding it (though still not particularly enamored with the idea to call a full subgroup lattice for the sake of complements).

@fingolfin fingolfin merged commit 79b83e1 into gap-system:master Nov 24, 2016
@hungaborhorvath hungaborhorvath deleted the ComplementClassesRepresentatives branch November 24, 2016 17:57
hungaborhorvath added a commit to hungaborhorvath/gap that referenced this pull request Nov 26, 2016
!!! 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.
hungaborhorvath added a commit to hungaborhorvath/gap that referenced this pull request Nov 29, 2016
!!! 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.
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
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 release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants