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

Improve performance of ConjugacyClasses for solvable groups #3031

Merged

Conversation

fingolfin
Copy link
Member

.. and use it in two ConjugacyClasses methods: the generic one
for finite groups, and the one for permutation groups. Also merge
the two existing "generic" methods for finite groups (they had the
same rank, which is rather brittle).

This speeds up the computation of ConjugacyClasses for e.g.

G:=WreathProduct(CyclicGroup(IsPermGroup,12), DihedralGroup(IsPermGroup,12));

from 110 seconds to 53 seconds on my laptop.

.. and use it in two ConjugacyClasses methods: the generic one
for finite groups, and the one for permutation groups. Also merge
the two existing "generic" methods for finite groups (they had the
same rank, which is rather brittle).

This speeds up the computation of ConjugacyClasses for e.g.

    G:=WreathProduct(CyclicGroup(IsPermGroup,12), DihedralGroup(IsPermGroup,12));

from 110 seconds to 53 seconds on my laptop.
@fingolfin fingolfin added topic: performance bugs or enhancements related to performance (improvements or regressions) topic: library labels Nov 20, 2018
Copy link
Member

@markuspf markuspf left a comment

Choose a reason for hiding this comment

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

This looks like a sensible change to me. For the un-initiated it might be helpful to comment on the cost of proving solvability (CanEasilyComputePcgs is kind of self-explanatory).

@@ -3061,6 +3061,8 @@ local cl;
cl:=ConjugacyClassesForSmallGroup(G);
if cl<>fail then
return cl;
elif IsSolvableGroup( G ) and CanEasilyComputePcgs(G) then
Copy link
Member Author

Choose a reason for hiding this comment

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

I added the "raw" IsSolvableGroup check here because (a) it is reasonably efficient for permutation groups, and its result can be useful in many subsequent computations, too, and (b) I felt that the existing check for IsSimpleGroup later on is not really cheaper either. But of course I might be mistaken -- but I rely on @hulpke to point that out :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

For permutation groups IsSimpleGroup is not as expensive as it might seem, as it uses tools from the composition series mechanism. In most cases order alone bails out quickly.
IsSolvableGroup should be cheap enough, as the code running afterwards effectively would discover that the group is solvable.

[ IsGroup and IsFinite ],
function(G)
local cl;
cl:=ConjugacyClassesForSmallGroup(G);
if cl<>fail then
return cl;
elif IsSolvableGroup( G ) and CanEasilyComputePcgs(G) then
Copy link
Member Author

Choose a reason for hiding this comment

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

Here perhaps we should instead start with HasIsSolvableGroup( G ) and ..., as this code could be called on arbitrary groups. The only reason I didn't do it was that the existing method didn't do it either.

BTW, CanEasilyComputePcgs is a category, and set by some implications once the group "knows" that it is solvable. So the order of the check here is crucial.

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

If indeed the pure pcgs code is so much faster, this is a good addition. (My past reason for not linking it in was that I was more interested in having my new code be tested.)

@@ -3061,6 +3061,8 @@ local cl;
cl:=ConjugacyClassesForSmallGroup(G);
if cl<>fail then
return cl;
elif IsSolvableGroup( G ) and CanEasilyComputePcgs(G) then
Copy link
Contributor

Choose a reason for hiding this comment

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

For permutation groups IsSimpleGroup is not as expensive as it might seem, as it uses tools from the composition series mechanism. In most cases order alone bails out quickly.
IsSolvableGroup should be cheap enough, as the code running afterwards effectively would discover that the group is solvable.

@hulpke
Copy link
Contributor

hulpke commented Nov 20, 2018

(I think the time overhead in the code is purely with storing informations in the centralizers about how they fit with the chosen series -- no reason to not make this change but also not to worry about the other code. It is an artifect of the huge number of classes)

@fingolfin fingolfin added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Nov 21, 2018
@fingolfin fingolfin merged commit 75f11d9 into gap-system:master Nov 21, 2018
@fingolfin fingolfin deleted the mh/ConjugacyClassesForSolvableGroup branch November 21, 2018 14:17
@markusbaumeister markusbaumeister added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@fingolfin fingolfin changed the title Add ConjugacyClassesForSolvableGroup Improve performance of ConjugacyClasses for solvable groups Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants