Skip to content

Avoid IdGroup in IsSymmetricGroup - #6383

Merged
ThomasBreuer merged 2 commits into
masterfrom
mh/IdGroup3
May 9, 2026
Merged

Avoid IdGroup in IsSymmetricGroup#6383
ThomasBreuer merged 2 commits into
masterfrom
mh/IdGroup3

Conversation

@fingolfin

Copy link
Copy Markdown
Member

Based on experiments, just testing the order seemed to be fastest. Though of course it also depends on the representation used for the group.

With this many more packages pass test with gap --bare.

In my experiments the new code was never slower and sometimes faster than IdGroup. For example, in master (the three groups used here are the ones of order 720 which contains an A6 of index 2)

gap> G:=SmallGroup(720, 763);; for i in [1..1000] do IsSymmetricGroup(Group(GeneratorsOfGroup(G))); od; time;
271
gap> G:=SmallGroup(720, 764);; for i in [1..1000] do IsSymmetricGroup(Group(GeneratorsOfGroup(G))); od; time;
1212
gap> G:=SmallGroup(720, 765);; for i in [1..1000] do IsSymmetricGroup(Group(GeneratorsOfGroup(G))); od; time;
994

With this PR:

gap> G:=SmallGroup(720, 763);; for i in [1..1000] do IsSymmetricGroup(Group(GeneratorsOfGroup(G))); od; time;
273
gap> G:=SmallGroup(720, 764);; for i in [1..1000] do IsSymmetricGroup(Group(GeneratorsOfGroup(G))); od; time;
943
gap> G:=SmallGroup(720, 765);; for i in [1..1000] do IsSymmetricGroup(Group(GeneratorsOfGroup(G))); od; time;
715

Based on experiments, just testing the order seemed to be fastest.
Though of course it also depends on the representation used for
the group.
@fingolfin
fingolfin requested a review from ThomasBreuer May 8, 2026 11:13
@fingolfin fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels May 8, 2026

@ThomasBreuer ThomasBreuer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Iterating over the group looks horrible. What about stopping at the first element of order 6 (then we are sure the group is S6) or of order 8 or 10 (then we are sure the group is not S6)?

@codecov

codecov Bot commented May 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.69%. Comparing base (fc1bbdf) to head (ea50863).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
lib/grpnames.gi 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6383   +/-   ##
=======================================
  Coverage   78.69%   78.69%           
=======================================
  Files         684      684           
  Lines      292891   292897    +6     
  Branches     8686     8660   -26     
=======================================
+ Hits       230497   230505    +8     
+ Misses      60581    60577    -4     
- Partials     1813     1815    +2     

☔ View full report in Codecov by Sentry.
📢 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

Copy link
Copy Markdown
Member Author

@ThomasBreuer good idea, done now

@ThomasBreuer
ThomasBreuer merged commit 4e3501f into master May 9, 2026
33 checks passed
@ThomasBreuer
ThomasBreuer deleted the mh/IdGroup3 branch May 9, 2026 18:12
cdwensley pushed a commit that referenced this pull request May 12, 2026
cdwensley pushed a commit that referenced this pull request Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants