Symplectic C5 #75

TristanPfersdorff · 2021-10-31T14:21:41Z

Add the necessary functions for the C5 subgroups in the symplectic case. The codecov bot will be happy once we add the symplectic main function.

Edit: It appears that the checks have some really fishy issue due to RECOG, but I was only able to reproduce said issue once in a blue moon locally. Everything seems to work fine 99% of the time, so this must be some randomized-algorithm-stuff from RECOG.

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.

maxhauck · 2021-10-31T22:46:52Z

gap/ClassicalMaximals.gi

+
+    # For each prime divisor of e, there is exactly one of these subgroups,
+    # so this is sufficient.
+    return List(PrimeDivisors(e), b -> SubfieldSp(n, p, e, QuoInt(e ,b)));


You have to remember conjugating the subgroup you constructed by an element of GSp \ Sp in order to get all the conjugacy classes in Sp, see for example ll. 833-834 at https://github.com/hulpke/maxtrans/blob/master/magma/classmax.m

fingolfin

Some suggestions. Overall looks fine to me, modulo the remark by @maxhauck

fingolfin · 2021-11-03T12:32:36Z

gap/ClassicalMaximals.gi

+
+    # For each prime divisor of e, there is exactly one of these subgroups,
+    # so this is sufficient.
+    return List(PrimeDivisors(e), b -> SubfieldSp(n, p, e, QuoInt(e ,b)));


Suggested change

return List(PrimeDivisors(e), b -> SubfieldSp(n, p, e, QuoInt(e ,b)));

return List(PrimeDivisors(e), b -> SubfieldSp(n, p, e, QuoInt(e, b)));

fingolfin · 2021-11-03T12:35:02Z

gap/SubfieldMatrixGroups.gi

+    local F, q0, b, gens, l, zeta, omega, zetaPower, C, gen;
+
+    if IsOddInt(d) then
+    	ErrorNoReturn("<d> must even but ", d, " was given.");


Suggested change

ErrorNoReturn("<d> must even but ", d, " was given.");

ErrorNoReturn("<d> must be even but ", d, " was given.");

However, a general note to both of you: it is not really necessary to print the value d here, as the user will be put on a break loop if the error is triggered, and so they can just enter d; followed by return to see the value, if they wanted to. So you could just do

Suggested change

ErrorNoReturn("<d> must even but ", d, " was given.");

ErrorNoReturn("<d> must be even");

fingolfin · 2021-11-03T12:36:49Z

gap/SubfieldMatrixGroups.gi

+    if e mod f <> 0 or not IsPrime(b) then
+        ErrorNoReturn("<f> must be a divisor of <e> and their quotient must be a prime but <e> = ", 
+                      e, " and <f> = ", f);
+    fi;


In general I'd try move input validation to the very start, before any work is done. Also, this checks for two conditions, and so I'd suggest to have two errors; that makes it slightly easier for a human to understand what's going on. I.e., together with what I wrote above:

Suggested change

if e mod f <> 0 or not IsPrime(b) then

ErrorNoReturn("<f> must be a divisor of <e> and their quotient must be a prime but <e> = ",

e, " and <f> = ", f);

fi;

if e mod f <> 0 then

ErrorNoReturn("<f> must be a divisor of <e>");

fi;

if not IsPrime(b) then

ErrorNoReturn("The quotient of <f> by <e> must be a prime");

fi;

fingolfin · 2021-11-03T12:38:58Z

gap/SubfieldMatrixGroups.gi

+    # This matrix C preserves the form and is constructed to
+    # have determinant 1, but it is not in Sp(d, q0). Therefore it is
+    # our missing generator to extend Sp(d, q0) to a C5-subgroup, since
+    # C is in the Normalizer of Sp(d, q) of Sp(d, q0).


Here q0 is a divisor of q, so I am assuming you mean the normalizer N_{Sp(d,q)}(Sp(d,q0)); the following phrasing describes it more clearly, IMHO:

Suggested change

# C is in the Normalizer of Sp(d, q) of Sp(d, q0).

# C is in the normalizer of Sp(d, q0) in Sp(d, q).

fingolfin · 2021-11-03T12:40:01Z

gap/SubfieldMatrixGroups.gi

+    # have determinant 1, but it is not in Sp(d, q0). Therefore it is
+    # our missing generator to extend Sp(d, q0) to a C5-subgroup, since
+    # C is in the Normalizer of Sp(d, q) of Sp(d, q0).
+    C := DiagonalMat(Concatenation(List([1..l], i -> omega * zetaPower), List([1..l], i -> zetaPower)));


Suggested change

C := DiagonalMat(Concatenation(List([1..l], i -> omega * zetaPower), List([1..l], i -> zetaPower)));

C := DiagonalMat(Concatenation(ListWithIdenticalEntries(l, omega * zetaPower), ListWithIdenticalEntries(l, zetaPower)));

TristanPfersdorff · 2021-11-03T17:11:59Z

The proposed changes seem good to me, I have implemented them and rebased. As for the remark by @maxhauck, it seems like it suffices to call ConjugatesInGeneralGroup with his function NormSpMinusSp from the C6-PR (#74) in the right spot once that PR has been merged (maybe one could do some fancy git-stuff with this PR depending on #74, but I'm no expert).
I still don't really know what to make of that weird RECOG-error (Error, Matrix INV: <mat> must be square (not 2 by 4)); It seems to happen somewhat frequently when the test run for the first time, but it rarely occurs when re-running the tests (without reloading the package). I've encountered it a couple times now but I can't seem to make sense of it, since every matrix we use is square.

fingolfin

As with PR #74 this now has a conflict in gap/ClassicalMaximals.gi so this PR here can't be merged; also once PR #74 is merged we may have a fresh conflict. So yeah, perhaps move the C1/C2/... cases into separate files?

Also, tests are failing, that needs to be resolved first (I overlooked that).

fingolfin · 2021-11-10T14:27:46Z

tst/standard/SubfieldMatrixGroups.tst

+gap> testsSubfieldSp := [[6, 2, 2, 1], [4, 3, 2, 1], [4, 3, 4, 2], [4, 7, 2, 1]];;
+gap> ForAll(testsSubfieldSp, TestSubfieldSp);
+true
+gap> SubfieldSp(3, 2, 2, 1);


I would suggest to add a comment that explains error handling is being tested (as opposed to a broken test file)

Suggested change

gap> SubfieldSp(3, 2, 2, 1);

# test error handling

gap> SubfieldSp(3, 2, 2, 1);

codecov · 2021-11-17T12:52:07Z

Codecov Report

Merging #75 (0cb7f16) into main (9632197) will decrease coverage by 0.62%.
The diff coverage is 75.75%.

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
- Coverage   91.63%   91.01%   -0.63%     
==========================================
  Files          14       14              
  Lines        2415     2504      +89     
==========================================
+ Hits         2213     2279      +66     
- Misses        202      225      +23

Impacted Files	Coverage Δ
gap/ClassicalMaximals.gi	`84.35% <0.00%> (-2.01%)`	⬇️
gap/SubfieldMatrixGroups.gi	`78.49% <100.00%> (+3.33%)`	⬆️
gap/TensorInducedMatrixGroups.gi	`90.74% <0.00%> (-2.78%)`	⬇️
gap/SemilinearMatrixGroups.gi	`92.71% <0.00%> (-2.44%)`	⬇️

maxhauck reviewed Oct 31, 2021

View reviewed changes

fingolfin reviewed Nov 3, 2021

View reviewed changes

TristanPfersdorff force-pushed the tp/symplectic-c5 branch from 7b3bb12 to 11d3ee9 Compare November 3, 2021 17:03

TristanPfersdorff added 7 commits November 6, 2021 16:34

Constructor for C5 (broken)

0fc1ee5

Fix some bugs

9549861

Generic function

cd6a743

Add template for tests

47352b4

Add tests for construction function

4ba7bc5

Implement suggestions by @fingolfin

6698815

Add tests for invalid inputs

9f4c917

TristanPfersdorff force-pushed the tp/symplectic-c5 branch from dbb4587 to 9f4c917 Compare November 6, 2021 15:34

fingolfin approved these changes Nov 10, 2021

View reviewed changes

fingolfin requested changes Nov 10, 2021

View reviewed changes

fingolfin reviewed Nov 10, 2021

View reviewed changes

TristanPfersdorff mentioned this pull request Nov 10, 2021

Symplectic C2 #77

Merged

7 tasks

fingolfin added 2 commits November 17, 2021 09:26

Merge branch 'main' into tp/symplectic-c5

764a6fa

Merge branch 'main' into tp/symplectic-c5

0cb7f16

fingolfin closed this Nov 17, 2021

fingolfin reopened this Nov 17, 2021

fingolfin merged commit c72b2bd into gap-packages:main Nov 18, 2021

TristanPfersdorff deleted the tp/symplectic-c5 branch December 23, 2021 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symplectic C5 #75

Symplectic C5 #75

TristanPfersdorff commented Oct 31, 2021 •

edited

Loading

maxhauck Oct 31, 2021

fingolfin left a comment

fingolfin Nov 3, 2021

fingolfin Nov 3, 2021

fingolfin Nov 3, 2021

fingolfin Nov 3, 2021

fingolfin Nov 3, 2021

TristanPfersdorff commented Nov 3, 2021

fingolfin left a comment

fingolfin Nov 10, 2021

codecov bot commented Nov 17, 2021

	return List(PrimeDivisors(e), b -> SubfieldSp(n, p, e, QuoInt(e ,b)));
	return List(PrimeDivisors(e), b -> SubfieldSp(n, p, e, QuoInt(e, b)));

	ErrorNoReturn("<d> must even but ", d, " was given.");
	ErrorNoReturn("<d> must be even but ", d, " was given.");

	ErrorNoReturn("<d> must even but ", d, " was given.");
	ErrorNoReturn("<d> must be even");

-    if e mod f <> 0 or not IsPrime(b) then
-        ErrorNoReturn("<f> must be a divisor of <e> and their quotient must be a prime but <e> = ",
-                      e, " and <f> = ", f);
-    fi;
+    if e mod f <> 0 then
+        ErrorNoReturn("<f> must be a divisor of <e>");
+    fi;
+    if not IsPrime(b) then
+        ErrorNoReturn("The quotient of <f> by <e> must be a prime");
+    fi;

	# C is in the Normalizer of Sp(d, q) of Sp(d, q0).
	# C is in the normalizer of Sp(d, q0) in Sp(d, q).

	C := DiagonalMat(Concatenation(List([1..l], i -> omega * zetaPower), List([1..l], i -> zetaPower)));
	C := DiagonalMat(Concatenation(ListWithIdenticalEntries(l, omega * zetaPower), ListWithIdenticalEntries(l, zetaPower)));

Symplectic C5 #75

Symplectic C5 #75

Conversation

TristanPfersdorff commented Oct 31, 2021 • edited Loading

Checklist for the reviewer

General

Functions constructing generators of maximal subgroups

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

maxhauck Oct 31, 2021

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Nov 3, 2021

Choose a reason for hiding this comment

fingolfin Nov 3, 2021

Choose a reason for hiding this comment

fingolfin Nov 3, 2021

Choose a reason for hiding this comment

fingolfin Nov 3, 2021

Choose a reason for hiding this comment

fingolfin Nov 3, 2021

Choose a reason for hiding this comment

TristanPfersdorff commented Nov 3, 2021

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Nov 10, 2021

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2021

Codecov Report

TristanPfersdorff commented Oct 31, 2021 •

edited

Loading