-
Notifications
You must be signed in to change notification settings - Fork 8
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
Start of almost simple in defining characteristic #41
base: main
Are you sure you want to change the base?
Start of almost simple in defining characteristic #41
Conversation
Codecov Report
@@ Coverage Diff @@
## main #41 +/- ##
===========================================
- Coverage 81.95% 69.65% -12.30%
===========================================
Files 14 15 +1
Lines 2028 2432 +404
===========================================
+ Hits 1662 1694 +32
- Misses 366 738 +372
|
BindGlobal("l3qdim6", | ||
function(q) | ||
local A, G, M, MM, S, T, general, w; | ||
general := ValueOption("general"); |
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.
The ValueOption
abomination should be replaced by adding a variadic argument. This whole option business accesses and modifies a global state, from the docs 8 Options Stack
: "GAP supports a global options system".
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 does it even have to be variadic? Can't this function simply have two arguments, the second being general
,l and people are free to pass in false
for it?
BTW, while I agree options are problematic, they are often used to substitute for a feature GAP sadly is lacking: keyword arguments. In GAP you can end up with function calls like func(true, true, false)
that can be rather confusing, compared to something like func(general:=true, checks:=true, format_drive:=false)
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.
I guess what I tried to say with my last remark is: dang, we really should have keyword arguments in GAP ;-). I once started working on that but quickly decided to put it aside, because while I think implementing it would be fun, I really should be spending my time on other things sigh. But if anybody is ever interested in diving into the GAP kernel and implementing some language extensions, I'd be happy to teach you the ropes.
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.
I guess what I tried to say with my last remark is: dang, we really should have keyword arguments in GAP ;-).
That would be a dream come true. :D
I had the impression that, at least in the magma version, these functions share a lot of code. It may make sense to have one very abstract function, which then delegates to more specialized functions. E.g. I think many of the magma functions first build a tensor module "V tensor V", then compute the constituents, and then looks for a constituent with a given dimension. The only thing that changes between this part of these functions is the dimension we're looking for. |
@@ -0,0 +1,26 @@ | |||
# Construction of group as in Table 5.6 row 4 from [BHR13] | |||
BindGlobal("l3qdim6", |
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.
Sorry for this nitpick, but:
That's a rather cryptic name... I have no idea what this function is supposed to compute. Maybe at least add a comment, perhaps we can also find a slightly more helpful name? (Also, global variables/functions in GAP normally start with an uppercase letter?)
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.
Yepp, the current name is taken from magma. The abstract function could have a name like ConstructDefiningCharacteristicRepresentationOfAlmostSimpleGroup
, that's what's happening here. The function l3qdim6
constructs one of these embeddings, namely the representation of an SL_3(q) on the symmetric-square module of dimension 6. There's a lot of these constructions which are extremely similar, which is why I'm suggesting something like ConstructDefiningCharacteristicRepresentationOfAlmostSimpleGroup
or maybe ConstructDefiningCharacteristicRepresentationOfAlmostSimpleGroupOnSymmetricPowerModule
SU3 Dim 6 Fixes
32d9e46
to
fa288d0
Compare
# needs to load the Recog Package. | ||
|
||
|
||
# From the Recog Package but slightly modified (by DR). |
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.
Did you copy, paste that? Could we instead extend the function in recog such that you can also use it here?
Any news on this? Would it help / make sense to meet to discuss this? |
Anna and I haven't had time to continue working on the project until now. Hopefully we'll get back to it during the semester break. |
Start of a file for the almost simple groups in defining characteristic (chapter 5 in the book by colva). In this PR we are also adding two functions to show how these groups can be generated in GAP.
Checklist for the reviewer
General
Functions constructing generators of maximal subgroups
RECOG.TestGroup
from the recog package.DefaultFieldOfMatrixGroup
returns the correct field.Functions assembling the list of all maximal subgroups of a certain group
The reviewer doesn't need to compare our results to magma's results. That's the job of the person implementing the code.