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

Fix missing outer automorphisms of certain Chevalley groups of type O_n-(q), n>=8 even; this also fixes calculation of certain normalizers in S_n #4320

Merged
merged 2 commits into from
Mar 18, 2021

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Mar 15, 2021

Instead of tables in ATLAS (that had been misinterpreted), use the
explict presentations in Bray/Holt/Roney-Dougal.

This can lead to wrong results when calculating normalizers in S_n.

The groups affected are of type O_n-(q), n>=8 even
(and to a lesser extent -- wrong outer automorphism group structure, but that is not used in other places for deductions -- S4(4^e) )

This fixes #4318

@hulpke hulpke added kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes priority: high backport-to-4.11 labels Mar 15, 2021
@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #4320 (9d9e754) into master (282d520) will decrease coverage by 57.71%.
The diff coverage is 55.55%.

@@             Coverage Diff             @@
##           master    #4320       +/-   ##
===========================================
- Coverage   93.51%   35.79%   -57.72%     
===========================================
  Files         716      659       -57     
  Lines      812023   783590    -28433     
===========================================
- Hits       759366   280498   -478868     
- Misses      52657   503092   +450435     
Impacted Files Coverage Δ
grp/simple.gi 78.00% <55.55%> (-3.08%) ⬇️
src/funcs.h 0.00% <0.00%> (-100.00%) ⬇️
grp/perf23.grp 4.65% <0.00%> (-95.35%) ⬇️
grp/perf21.grp 5.36% <0.00%> (-94.64%) ⬇️
grp/perf20.grp 5.82% <0.00%> (-94.18%) ⬇️
grp/perf17.grp 6.36% <0.00%> (-93.64%) ⬇️
grp/perf18.grp 6.66% <0.00%> (-93.34%) ⬇️
grp/perf22.grp 7.84% <0.00%> (-92.16%) ⬇️
grp/perf15.grp 8.32% <0.00%> (-91.68%) ⬇️
grp/perf16.grp 8.52% <0.00%> (-91.48%) ⬇️
... and 404 more

@hulpke hulpke force-pushed the fixes branch 3 times, most recently from 69f9995 to 41e4737 Compare March 16, 2021 18:31
Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Thanks for making this fix @hulpke.

Unsurprisingly the failing 32-bit GitHub Actions job checks is nothing to do with you. But the failure means that I at least won't be able to merge this until it's working again. Someone with greater permissions should be able to, however.

grp/simple.gi Outdated
Comment on lines 906 to 907
rels:=Concatenation("d",String(Gcd(q+1,n)),
"=g2=1,f",String(e),"=g,d^g=D,d^f=d",String(p));
Copy link
Member

Choose a reason for hiding this comment

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

This is not a request to make any changes, just a note in case you're not aware: there is a fairly-new function StringFormatted in GAP, which I now tend to use in situations like this, where I'm constructing a string from various parts. Depending on the context and the code in question, it can sometimes make the result more readable (and yes, it can sometimes make the result less readable).

Suggested change
rels:=Concatenation("d",String(Gcd(q+1,n)),
"=g2=1,f",String(e),"=g,d^g=D,d^f=d",String(p));
rels:=StringFormatted("d{}=g2=1, f{}=g, d^g=D, d^f=d{}", Gcd(q+1,n), e, p);

Copy link
Contributor Author

@hulpke hulpke Mar 17, 2021

Choose a reason for hiding this comment

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

Thank you -- indeed that will make it much nicer. I will change this later today once I'm done with classes. [edit: done now]

@@ -887,9 +887,101 @@ local id;
return DataAboutSimpleGroup(id);
end);

# Tables for outer automorphisms from Bray/Holt/Roney-Dougal, p. 36/37
Copy link
Member

@wilfwilson wilfwilson Mar 17, 2021

Choose a reason for hiding this comment

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

Thanks for including this line.

I have no idea how we deal with referencing in GAP, in general (and whether we have any kind of 'policy' about it): so for anyone who knows about this sort of stuff, should there also be a corresponding reference to this book in the GAP manual/bibliography/website/somewhere else?

Copy link
Member

Choose a reason for hiding this comment

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

In practice there is no consistent way this is dealt with, I am afraid. Of course in principle it would be good to at least add a BibTeX entry for this in our master .bib file. Whether and where to cite it is a different question, though...

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Fine by me. Some minor questions

grp/simple.gi Outdated Show resolved Hide resolved
return String(Size(gp));
elif Size(gp)<=31 or Size(gp) in [33..47] then
s:=StructureDescription(gp);
s:=Filtered(s,x->not x in "C ");
Copy link
Member

Choose a reason for hiding this comment

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

What exactly is this test about? It seems to remove all spaces and all C from the structure description? The result won't be in stringrep, though. How about this instead?

Suggested change
s:=Filtered(s,x->not x in "C ");
s:=RemoveCharacters(s, "C ");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is about producing names for subgroups of outer automorphism groups that is consistent with what is used in the table of marks library (to find the group there). E.g. StructureDescription gives C2 x C2, while the table of marks (and character table) libraries use .2x2

nam:=Concatenation("O",String(par[1]*2),"-(",String(par[2]),")");
e:=EFactors(Gcd(4,par[2]^par[1]+1),expo/2,1);

e:=EFactors(Gcd(4,par[2]^par[1]+1),2*expo,1);
Copy link
Member

Choose a reason for hiding this comment

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

Why the change from /2 to *2 -- is that part of the bug fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That was the underlying bug. However this (slightly cryptic) variant is phased out now in favour of the presentations from Bray/Holt/Roney-Dougal, and currently both are checked against each other to avoid further implementation errors.

hulpke added 2 commits March 18, 2021 07:43
Instead of tables in ATLAS (that might have been misinterpreted), use the
explict presentations in Bray/Holt/Roney-Dougal.

This fixes gap-system#4318
@hulpke hulpke merged commit 52fa8c9 into gap-system:master Mar 18, 2021
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them topic: library release notes: use title For PRs: the title of this PR is suitable for direct use 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 backport-to-4.11 labels Aug 17, 2022
@fingolfin fingolfin changed the title FIX: Outer automorphisms of certain Chevalley groups. Fix missing outer automorphisms of certain Chevalley groups of type $O_n-(q)$, $n\geq 8$ even Aug 17, 2022
@fingolfin fingolfin changed the title Fix missing outer automorphisms of certain Chevalley groups of type $O_n-(q)$, $n\geq 8$ even Fix missing outer automorphisms of certain Chevalley groups of type O_n-(q), n>=8 even Aug 17, 2022
@fingolfin fingolfin changed the title Fix missing outer automorphisms of certain Chevalley groups of type O_n-(q), n>=8 even Fix missing outer automorphisms of certain Chevalley groups of type O_n-(q), n>=8 even; this also fixes calculation of certain normalizers in S_n Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them priority: high release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when computing automorphisms of simple orthogonal groups
3 participants