gap-packages / ClassicalMaximals

Maximal subgroups of classical groups

Other

0 stars 8 forks source link

Orthogonal C1 #106

Closed TristanPfersdorff closed 2 years ago

TristanPfersdorff commented 2 years ago

All the stuff to finish C1 for case O, including the construction for the isotropic subspace stabilizers and the generic function.

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.
[ ] 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[bot] commented 2 years ago

Codecov Report

Merging #106 (5f093da) into main (586799a) will decrease coverage by 0.85%. The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
- Coverage   93.65%   92.79%   -0.86%     
==========================================
  Files          14       14              
  Lines        3355     3552     +197     
==========================================
+ Hits         3142     3296     +154     
- Misses        213      256      +43

Impacted Files	Coverage Δ
gap/ClassicalMaximals.gi	`82.38% <2.38%> (-4.83%)`	:arrow_down:
gap/Utils.gi	`93.55% <93.33%> (-0.26%)`	:arrow_down:
gap/ReducibleMatrixGroups.gi	`98.78% <100.00%> (+0.31%)`	:arrow_up:
gap/Utils.gd	`100.00% <100.00%> (ø)`

fingolfin commented 2 years ago

@TristanPfersdorff Is this ready for merging now, from your POV? Or do you plan further changes?

TristanPfersdorff commented 2 years ago

Well, I have been working on a nice function AlternativeGeneratorsOfOrthogonalGroup in my C2-PR which abstracts away some of the slightly ugly code in this PR (the stuff with the discriminants in the NonDegenerate case with epsilon = 0) by constructing nice generators for the diagonal forms in case q odd, so I do plan to replace the ugly code by that function at some point down the line. But I don't think that is a good reason to block this PR, so I'd say this PR is ready for merging.