Symplectic C3

[maxhauck]

The same note as in #74 applies: 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]

Codecov Report

Merging #76 (601ccc9) into main (423a270) will decrease coverage by 1.10%.
The diff coverage is 73.78%.

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
- Coverage   92.17%   91.06%   -1.11%     
==========================================
  Files          14       14              
  Lines        2275     2474     +199     
==========================================
+ Hits         2097     2253     +156     
- Misses        178      221      +43     

Impacted Files	Coverage Δ
gap/ClassicalMaximals.gi	84.90% <7.69%> (-5.06%)	⬇️
gap/SemilinearMatrixGroups.gi	92.71% <87.50%> (-2.44%)	⬇️
gap/ImprimitiveMatrixGroups.gi	97.04% <100.00%> (+1.84%)	⬆️
gap/TensorProductMatrixGroups.gi	96.29% <100.00%> (+2.26%)	⬆️
gap/TensorInducedMatrixGroups.gi	90.74% <0.00%> (-2.78%)	⬇️

[fingolfin]

I think this could be improved somewhat. Let me rephrase what you do: you are looping over all even divisors d (= n/s) of n and then do something with n/d (=n/(n/s)=s). But the even divisors of n either don't exist (if n is odd) or they are equal to the divisors of n/2, each multiplied by 2.

[fingolfin]

Actually, here of course you only look at the prime divisors, so basically all you need is to let s iterate over the odd prime divisors. So unless I am missing something, one could do:

Suggested change

	primeDivisorsOfn := PrimeDivisors(n);
	result := [];
	# symplectic type subgroups
	for s in primeDivisorsOfn do
	if IsEvenInt(n / s) then
	Add(result, SymplecticSemilinearSp(n, q, s));
	fi;
	od;
	if not IsEvenInt(n) then return []; fi;
	primeDivisorsOfn := PrimeDivisors(n);
	RemoveSet(primeDivisorsOfn, 2)
	result := [];
	# symplectic type subgroups
	for s in primeDivisorsOfn do
	Add(result, SymplecticSemilinearSp(n, q, s));
	od;

However, my original remark still applies to the C2 case in PR #77.

[maxhauck]

Be careful here: If n is divisible by 4, then s = 2 is still admissible (as n / s = n / 2 is still even) - so you can't just throw out s = 2 entirely.

Of course, you could add an if-statement saying "throw out s = 2 if n is not divisible by 4, otherwise leave it in", but I think then the new syntax would not only be not much easier or clearer than the original one, but it would also obfuscate the intention behind it all (namely, that we just need to ensure that n / s is even).

[fingolfin]

Suggested change

	ErrorNoReturn("<s> must be prime and a divisor of <d> but <s> = ", s,
	" and <d> = ", d);
	ErrorNoReturn("<s> must be prime and a divisor of <d>");

I don't think printing s and d is necessary here, as the user can just query them via the break loop once they hit that error, no?

[maxhauck]

Well, so far, we always printed the relevant arguments in error messages - so you're suggesting to alter this paradigm?

[fingolfin]

Yes, I think I also suggested this in other PRs before. i can also turn this into an issue if you prefer. The way I suggest is the usual way to do this in GAP

[fingolfin]

At this point, As is not even defined. Perhaps the line setting As should be moved before this comment? Or perhaps say something like

Suggested change

	# Let w be a primitive element of GF(q ^ s) over GF(q). Since As is the
	# companion matrix of the minimal polynomial of w over GF(q), its
	# Let w be a primitive element of GF(q ^ s) over GF(q). The matrix As we define
	# next is the companion matrix of the minimal polynomial of w over GF(q), hence its

[fingolfin]

see above

[maxhauck]

We have written comments explaining newly defined objects above the relevant object all over the code - e.g., there are multiple more instances of this in the SemilinearMatrixGroups.gi. Personally, I find it more logical to have a comment above the relevant code instead of below ... - of course, if you feel strongly about this, we can also change it.

[fingolfin]

Having the comment above is fine by me but then it must be clear that it references things below it, because we read code from top to bottom, not the other way around. For a 1 line comment this is always fine, for large comments less so. And here it mixes a reference to an object not yet defined with claims about that object not immediately clear from the surrounding code (one has to look at a function call before the comment and look at that function resp. its docs/comments