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

Symplectic C6 #74

Merged
merged 6 commits into from
Nov 10, 2021
Merged

Symplectic C6 #74

merged 6 commits into from
Nov 10, 2021

Conversation

maxhauck
Copy link
Collaborator

@maxhauck maxhauck commented Oct 30, 2021

Add functions constructing C6 subgroups of Sp(n, q) and a corresponding generic function in the ClassicalMaximals.gi

Note: Lines not covered by the tests in ExtraspecialNormalizerMatrixGroups.gi are exclusively due to problems with RECOG, but I have checked these manually; the changes in ClassicalMaximals.gi will be tested once the symplectic main function is complete and we implement its tests.

Checklist for the reviewer

General

  • All functions have unit tests

Functions constructing generators of maximal subgroups

  • The code which computes and then stores the sizes is correct. To do this one confers to the tables referenced by the code. Tests for the group size should use RECOG.TestGroup from the recog package.
  • The test suite checks whether all constructed generators are sensible. That is it tests the generators'
    • determinants (and if applicable also the spinor norms are correct), and
    • compatibility with the correct form,
    • DefaultFieldOfMatrixGroup returns the correct field.

Functions assembling the list of all maximal subgroups of a certain group

  • The code agrees with the tables in section 8.2 of the book "The Maximal Subgroups of the Low-dimensional Finite Classical Groups".

The reviewer doesn't need to compare our results to magma's results. That's the job of the person implementing the code.

@codecov
Copy link

codecov bot commented Oct 30, 2021

Codecov Report

Merging #74 (168afa9) into main (be3b551) will decrease coverage by 1.38%.
The diff coverage is 62.88%.

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
- Coverage   93.64%   92.26%   -1.39%     
==========================================
  Files          14       14              
  Lines        2141     2223      +82     
==========================================
+ Hits         2005     2051      +46     
- Misses        136      172      +36     
Impacted Files Coverage Δ
gap/ClassicalMaximals.gi 89.96% <6.06%> (-5.49%) ⬇️
gap/ExtraspecialNormalizerMatrixGroups.gi 94.75% <92.06%> (-0.85%) ⬇️
gap/ClassicalMaximals.gd 100.00% <100.00%> (ø)

@fingolfin
Copy link
Member

Is the error in recog you are alluding to the same as gap-packages/recog#295 ?

gap/ClassicalMaximals.gi Show resolved Hide resolved
# According to the Magma code, there is no need to take another
# generator instead of U1 if we cannot rescale it to determinant 1 - we
# simply discard U1 as a generator.
# Comparing with the Magma code, we can simply take
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean here? Are you saying: "The Magma code does this, so we use this as justification for doing the same"? (that doesn't seem like a great argument, by the way ;-).

Or are you saying: "compared to the Magma code, we are better/different/whatever, and hence it suffices to do XYZ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The former :D ... well, sadly the relevant paper does not really say much (or rather anything) concerning this particular case so I trusted the Magma code on this one - however, doing this did pass some sanity checks: (1) the group sizes do come out the right way and (2) this case is exactly where an exception occurs in the structure of these C6 subgroups according to the tables in [BHR13] (namely, they are a bit smaller), so it makes sense that this should suffice.

One could of course check "theoretically" that these generators are enough, but I am not sure if this maybe transcends our current mathematical horizon a bit ;)

@TristanPfersdorff TristanPfersdorff mentioned this pull request Nov 3, 2021
7 tasks
@maxhauck
Copy link
Collaborator Author

maxhauck commented Nov 3, 2021

Is the error in recog you are alluding to the same as gap-packages/recog#295 ?

Ummmm, I honestly don't know, but could be ... - the tests just produced a ton of RECOG errors, which we currently handle by simply removing the error-causing tests until there are no errors anymore :D ... sadly, this means that, sometimes, not many tests are left :/

@maxhauck
Copy link
Collaborator Author

maxhauck commented Nov 5, 2021

I guess I can merge this, @fingolfin?

@fingolfin
Copy link
Member

  • the tests just produced a ton of RECOG errors, which we currently handle by simply removing the error-causing tests until there are no errors anymore

That's bad! Please at least turn those into bug reports for recog. Else they won't (can't!!) be fixed

@maxhauck maxhauck mentioned this pull request Nov 6, 2021
7 tasks
@fingolfin
Copy link
Member

There is a conflict now, which looks easy to resolve (basically two PRs were appending to the end of gap/ClassicalMaximals.gi). But I wonder if perhaps it would make sense to move the various cases C1, C3, etc. into separate source files, and only leave truly common stuff in gap/ClassicalMaximals.gi?

@fingolfin
Copy link
Member

I've manually resolved the conflict (via the web interface -- nice feature!) Assuming tests pass, we can merge it next.

@fingolfin fingolfin merged commit 87abe5b into gap-packages:main Nov 10, 2021
@maxhauck maxhauck deleted the mha/symplectic-c6 branch November 12, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants