-
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
Symplectic C3 #76
Symplectic C3 #76
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -194,3 +194,85 @@ function(d, q, s) | |||||||||
# conjugate the result so that it preserves the standard unitary form | ||||||||||
return ConjugateToStandardForm(MatrixGroupWithSize(F, generators, size), "U"); | ||||||||||
end); | ||||||||||
|
||||||||||
# Construction as in Proposition 6.4 of [HR05] | ||||||||||
BindGlobal("SymplecticSemilinearSp", | ||||||||||
function(d, q, s) | ||||||||||
local F, gammaL1, As, Bs, m, omega, AandB, C, i, range, generators, size; | ||||||||||
if d mod s <> 0 or not IsPrime(s) then | ||||||||||
ErrorNoReturn("<s> must be prime and a divisor of <d> but <s> = ", s, | ||||||||||
" and <d> = ", d); | ||||||||||
Comment on lines
+203
to
+204
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I don't think printing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, so far, we always printed the relevant arguments in error messages - so you're suggesting to alter this paradigm? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||
fi; | ||||||||||
if not IsEvenInt(QuoInt(d, s)) then | ||||||||||
ErrorNoReturn("The quotient <d> / <s> must be even but <d> = ", d, | ||||||||||
" and <s> = ", s); | ||||||||||
fi; | ||||||||||
F := GF(q); | ||||||||||
gammaL1 := MatricesInducingGaloisGroupOfGFQToSOverGFQ(s, q); | ||||||||||
# 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 | ||||||||||
Comment on lines
+212
to
+213
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point,
Suggested change
|
||||||||||
# determinant is (-1) ^ s times the constant term of said minimal | ||||||||||
# polynomial. By Vieta, this constant term is (-1) ^ s * the product of | ||||||||||
# all Galois conjugates of w. Hence, det(As) = w ^ ((q ^ s - 1) / (q - 1)). | ||||||||||
As := gammaL1.A; | ||||||||||
# By Lemma 6.2 det(Bs) = (-1) ^ (s - 1). | ||||||||||
Bs := gammaL1.B; | ||||||||||
m := QuoInt(d, s); | ||||||||||
|
||||||||||
omega := PrimitiveElement(GF(q ^ s)); | ||||||||||
AandB := List(GeneratorsOfGroup(Sp(m, q ^ s)), g -> MapGammaLToGL(g, As, omega)); | ||||||||||
|
||||||||||
C := IdentityMat(d, F); | ||||||||||
for i in [0..m - 1] do | ||||||||||
range := [i * s + 1..(i + 1) * s]; | ||||||||||
C{range}{range} := Bs; | ||||||||||
od; | ||||||||||
|
||||||||||
generators := Concatenation(AandB, [C]); | ||||||||||
# Size according to Table 2.6 of [BHR13] | ||||||||||
size := SizeSp(m, q ^ s) * s; | ||||||||||
# conjugate the result so that it preserves the standard symplectic form | ||||||||||
return ConjugateToStandardForm(MatrixGroupWithSize(F, generators, size), "S"); | ||||||||||
end); | ||||||||||
|
||||||||||
# Construction as in Proposition 6.5 of [HR05] | ||||||||||
BindGlobal("UnitarySemilinearSp", | ||||||||||
function(d, q) | ||||||||||
local F, gammaL1, A2, B2, omega, AandB, i, m, C, j, range, generators, size; | ||||||||||
if d mod 2 <> 0 then | ||||||||||
ErrorNoReturn("<d> must be divisible by 2 but <d> = ", d); | ||||||||||
fi; | ||||||||||
if q mod 2 = 0 then | ||||||||||
ErrorNoReturn("<q> must be odd but <q> = ", q); | ||||||||||
fi; | ||||||||||
|
||||||||||
F := GF(q); | ||||||||||
gammaL1 := MatricesInducingGaloisGroupOfGFQToSOverGFQ(2, q); | ||||||||||
# Let w be a primitive element of GF(q ^ 2) over GF(q). Since A2 is the | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||
# companion matrix of the minimal polynomial of w over GF(q), its | ||||||||||
# determinant is (-1) ^ 2 times the constant term of said minimal | ||||||||||
# polynomial. By Vieta, this constant term is (-1) ^ 2 * the product of | ||||||||||
# all Galois conjugates of w. Hence, det(A2) = w ^ ((q ^ s - 1) / (q - 1)) | ||||||||||
# = w ^ (q + 1). | ||||||||||
A2 := gammaL1.A; | ||||||||||
# By Lemma 6.2 det(B2) = (-1) ^ (s - 1) = -1. | ||||||||||
B2 := gammaL1.B; | ||||||||||
|
||||||||||
omega := PrimitiveElement(GF(q ^ 2)); | ||||||||||
AandB := List(GeneratorsOfGroup(GU(d / 2, q)), g -> MapGammaLToGL(g, A2, omega)); | ||||||||||
|
||||||||||
# Choose i such that (q + 1) / 2 ^ i is odd | ||||||||||
i := PValuation(q + 1, 2); | ||||||||||
# Note that det(A2 ^ m) = -1, i.e. det(B2 * A2 ^ m) = 1 | ||||||||||
m := (q ^ 2 - 1) / (2 ^ (i + 1)); | ||||||||||
C := IdentityMat(d, F); | ||||||||||
for j in [0..d / 2 - 1] do | ||||||||||
C{[2 * j + 1, 2 * j + 2]}{[2 * j + 1, 2 * j + 2]} := B2 * A2 ^ m; | ||||||||||
od; | ||||||||||
|
||||||||||
generators := Concatenation(AandB, [C]); | ||||||||||
# Size according to Table 2.6 of [BHR13] | ||||||||||
size := SizeGU(d / 2, q) * 2; | ||||||||||
# conjugate the result so that it preserves the standard symplectic form | ||||||||||
return ConjugateToStandardForm(MatrixGroupWithSize(F, generators, size), "S"); | ||||||||||
end); |
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 think this could be improved somewhat. Let me rephrase what you do: you are looping over all even divisors
d
(=n/s
) ofn
and then do something withn/d
(=n/(n/s)
=s
). But the even divisors ofn
either don't exist (ifn
is odd) or they are equal to the divisors ofn/2
, each multiplied by 2.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.
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:
However, my original remark still applies to the C2 case in PR #77.
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.
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).