Skip to content

Fix and enhancements to isomorphism test - #6544

Open
hulpke wants to merge 2 commits into
gap-system:masterfrom
hulpke:isom_better
Open

Fix and enhancements to isomorphism test#6544
hulpke wants to merge 2 commits into
gap-system:masterfrom
hulpke:isom_better

Conversation

@hulpke

@hulpke hulpke commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Ensure the subgroup found so far remains a subgroup of the search space. This fixes the bug from #6537.

Add a documented global function AsAutomorphism(sub,autom), replacing the
local asAutom/asAutomorphism helpers that were duplicated in
AutomorphismGroup and PatheticIsomorphism.

Factor the normal subgroup class stabilization into a local function
stabilizeSubgroupClass, using permutation action and set stabilizer
instead of OrbitStabilizerAlgorithm, lower the index threshold to 10^3 for
using it.

When finding the isomorphism through multiple orbits and stabilizers, reduce
the number of generators.

Clean out some dead code and careless double computation.

Avoid searching hard for special case of 2 generators with few images. if the cost seems high a priori — tune down the special case selection.

Release notes:

A bug in the isomorphism test for groups was fixed, overall performance of the test was improved.

@hulpke hulpke added kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes priority: high labels Aug 30, 2026
@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.26316% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.00%. Comparing base (989cc35) to head (5d6329e).

Files with missing lines Patch % Lines
lib/autsr.gi 86.36% 12 Missing ⚠️
lib/morpheus.gi 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6544      +/-   ##
==========================================
+ Coverage   78.98%   79.00%   +0.02%     
==========================================
  Files         684      684              
  Lines      294205   294246      +41     
  Branches     8647     8670      +23     
==========================================
+ Hits       232370   232480     +110     
+ Misses      60028    59959      -69     
  Partials     1807     1807              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fingolfin fingolfin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @hulpke . I'll try to review this in detail in the next days, but would appreciate if it was not merged before I had a chance to run some tests and understand what's going on.

Comment thread lib/autsr.gi
# nevertheless faster..

#AsIsomorphism
InstallGlobalFunction(AsAutomorphism,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That doesn't match our naming conventions, it should be, say,

Suggested change
InstallGlobalFunction(AsAutomorphism,
InstallGlobalFunction(OnSubgroupsByAutomorphism,

(The rds package already defines an OnSubgroups action, albeit a bit differently, so we can't just use that)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know that the name is nonstandard. But this action could be not just on subgroups, but on elements. Maybe OnGroupsAsAutomorphism?

Comment thread lib/autsr.gi
Comment thread lib/autsr.gi Outdated
@ThomasBreuer

Copy link
Copy Markdown
Contributor

With this pull request:

gap> G:= PcGroupCode( 2716742166652831895153946432, 448 );;
gap> PatheticIsomorphism( G, G );
Error, Record Element: '<rec>.makeaqiso' must have an assigned value
Stack trace:
*[1] origa!.makeaqiso(  );
   @ /.../lib/autsr.gi:2330
 [2] makenewa( Length( GeneratorsOfGroup( a ) ) > 12 );
   @ /.../lib/autsr.gi:2354
<function "PatheticIsomorphism">( <arguments> )
 called from read-eval loop at *stdin*:19
you can enter 'quit;' to quit to outer loop

hulpke and others added 2 commits August 31, 2026 11:30
Add a documented global function `AsAutomorphism(sub,autom)`, replacing the
local `asAutom`/`asAutomorphism` helpers that were duplicated in
`AutomorphismGroup` and `PatheticIsomorphism`.

Factor the normal subgroup class stabilization into a local function
`stabilizeSubgroupClass`, using permutation action and set stabilizer
instead of `OrbitStabilizerAlgorithm`, lower the index threshold to 10^3 for
using it.

Keep the inner automorphism group as permutations for the condition search.

When finding the isomorphism through multiple orbits and stabilizers, reduce
the number of generators.

Clean out some dead code and careless double computation.

Ensure existing permrep is used for stabilizer.

Avoid searching hard for 2 generators if the cost seems high.
@hulpke

hulpke commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author
gap> G:= PcGroupCode( 2716742166652831895153946432, 448 );;
gap> PatheticIsomorphism( G, G );
Error, Record Element: '<rec>.makeaqiso' must have an assigned value

Fixed. (But please note that PatheticIsomorphism is not a user function.)

@ThomasBreuer

Copy link
Copy Markdown
Contributor

Fixed. (But please note that PatheticIsomorphism is not a user function.)

Thanks a lot, now the code works.

Unfortunately, I get the following timings in a related example.

codes:= [ 14471979246740899087597746358515391646652811741626724220312361444709666170613060394496,
18407730836382948665918351628479071382519842228217209675805892885401437562320551430123222420888927068003840 ];;
grps:= List( codes, c -> PcGroupCode( c, 2^9*7 ) );;
G:= grps[1];;
H:= grps[2];;
IsomorphismGroups( G, H );
time;
  • GAP 4.16.0: 16,3 sec.
  • GAP 4.16.1: 15,7 sec.
  • current master branch: 45,8 sec.

The time increase seems to be due to a longer list of generators in the result of CompatiblePairs, which causes a larger number of LiftInduciblePair calls.

I hope that the longer generators list is not really needed. (In my examples, the actual results of IsomorphismGroups are correct in all three of the abovementioned versions.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-4.16 do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them priority: high release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants