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

regression in conjugacy tests for permutation groups? #5564

Closed
ThomasBreuer opened this issue Jan 8, 2024 · 3 comments · Fixed by #5620
Closed

regression in conjugacy tests for permutation groups? #5564

ThomasBreuer opened this issue Jan 8, 2024 · 3 comments · Fixed by #5620
Labels
regression A bug that only occurs in the branch, not in a release

Comments

@ThomasBreuer
Copy link
Contributor

The GAP input shown below needs about 11 seconds time and less than 200 MB space in GAP 4.12.2.
With the current master branch version, the default upper bound of 2 GB for the space is reached after 35 seconds.
If I do not prescribe this bound then the master branch version grows to 6 GB after 10 minutes, and still has not finished.

q:= 4;;  n:= 8;;
G:= DerivedSubgroup( SO( 1, n, q ) );;
points:= NormedRowVectors( GF(q)^n );;
orbs:= OrbitsDomain( G, points, OnLines );;
G:= Image( ActionHomomorphism( G, orbs[1], OnLines ) );;
repeat x:= Random( G ); until Order( x ) mod 13 = 0;
x:= x^( Order( x ) / 13 );;
c:= Centralizer( G, x );;
syl5:= SylowSubgroup( c, 5 );;
elms:= Filtered( Elements( syl5 ), y -> Order( y ) = 5 );;
reps:= Set( elms, SmallestGeneratorPerm );;
reps65:= List( reps, y -> SubgroupNC( G, [ y * x ] ) );;
pairs:= Combinations( reps65, 2 );;
res:= ForAny( pairs, p -> IsConjugate( G, p[1], p[2] ) );
@ThomasBreuer ThomasBreuer added the regression A bug that only occurs in the branch, not in a release label Jan 8, 2024
@fingolfin
Copy link
Member

According to git bisect, this regression was introduced in commit f64e305 via PR #5514 by @hulpke

hulpke added a commit to hulpke/gap that referenced this issue Jan 8, 2024
Do not try the orbit mapping test for cyclic subgroups (or subgroups with
many orbits). Thiscould be memory intensive, and is done reasonably by
backtrack.

This resolves gap-system#5564
@hulpke
Copy link
Contributor

hulpke commented Jan 8, 2024

Ah, for many short orbits the new code is inefficient, and cyclic subgroups actually are handled specially by the backtrack. I have corrected this.

hulpke added a commit to hulpke/gap that referenced this issue Jan 8, 2024
Do not try the orbit mapping test for cyclic subgroups (or subgroups with
many orbits). Thiscould be memory intensive, and is done reasonably by
backtrack.

This resolves gap-system#5564
@fingolfin
Copy link
Member

@hulpke it seems you have a potential fix ready, would you submit it as a PR?

hulpke added a commit to hulpke/gap that referenced this issue Feb 2, 2024
Do not try the orbit mapping test for cyclic subgroups (or subgroups with
many orbits). Thiscould be memory intensive, and is done reasonably by
backtrack.

This resolves gap-system#5564
fingolfin pushed a commit that referenced this issue Feb 2, 2024
…f an FPGroup which could result in wrong results (#5620)

* Conjugacy test for cyclic subgroups

Do not try the orbit mapping test for cyclic subgroups (or subgroups with
many orbits). Thiscould be memory intensive, and is done reasonably by
backtrack.

This resolves #5564

* FIX: Abelianized subgroup rewriting

Replace method for `MaximalAbelianQuotient` for a subgroup with an easier
version that rewrites the presentation and then abelianizes.

This fixes #5609

* ENHANCE: AGSRCharacteristicSeries refine

Only attempt refinements with PCore, Agemo, Omega, if steps are not already
elementary abelian. This can avoid expensive subcomputations.
hulpke added a commit to hulpke/gap that referenced this issue Mar 29, 2024
Do not try the orbit mapping test for cyclic subgroups (or subgroups with
many orbits). Thiscould be memory intensive, and is done reasonably by
backtrack.

This resolves gap-system#5564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression A bug that only occurs in the branch, not in a release
Projects
None yet
3 participants