-
Notifications
You must be signed in to change notification settings - Fork 163
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
Conversation
Codecov Report
@@ 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
|
69f9995
to
41e4737
Compare
There was a problem hiding this 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
rels:=Concatenation("d",String(Gcd(q+1,n)), | ||
"=g2=1,f",String(e),"=g,d^g=D,d^f=d",String(p)); |
There was a problem hiding this comment.
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).
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); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this 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
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 "); |
There was a problem hiding this comment.
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?
s:=Filtered(s,x->not x in "C "); | |
s:=RemoveCharacters(s, "C "); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Instead of tables in ATLAS (that might have been misinterpreted), use the explict presentations in Bray/Holt/Roney-Dougal. This fixes gap-system#4318
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