-
Notifications
You must be signed in to change notification settings - Fork 120
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
some methods for computing orth. discriminants #2748
some methods for computing orth. discriminants #2748
Conversation
- add `od_from_order`, `od_from_eigenvalues`, `od_for_specht_module - add utilities `character_of_entry`, `order_omega_mod_N`, `reduce_mod_squares`
Codecov Report
@@ Coverage Diff @@
## master #2748 +/- ##
==========================================
+ Coverage 73.20% 73.41% +0.21%
==========================================
Files 446 451 +5
Lines 63675 63886 +211
==========================================
+ Hits 46615 46905 +290
+ Misses 17060 16981 -79 |
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.
Fine by me, but some questions and suggestions -- but all optional, feel free to ignore some or all.
``` | ||
""" | ||
function reduce_mod_squares(F::AnticNumberField, val::nf_elem) | ||
@req parent(val) === F "val must have parent F" |
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.
Why is F
even an argument when the only place using it is this check?
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.
Yes.
I still have to get used to the fact that the elements carry the relevant context.
is_zero(val) && return val | ||
d = denominator(val) | ||
if ! isone(d) | ||
val = val * d^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.
val = val * d^2 | |
val *= d^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.
Do you want to say that this syntax is preferable?
If yes then should this be stated in the "Deveoper Style Guide"? (There are quite some places in the code where one could switch to this syntax.)
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.
From my POV either is fine, this is a matter of taste. Pick whichever you prefer :-)
val = val * d^2 | ||
end | ||
if is_integer(val) | ||
val = ZZ(val) |
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.
It may not matter, but just FYI, this breaks type stability (and I think possibly even of the whole function?) for val
if it is not already a ZZRingElem
.
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 do not understand this comment. In the end, an element of F
is returned.
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.
The code inside this function is unstable: Julia cannot determine a concrete type for val
anymore: val
was initially an nf_elem
, but now is a ZZRingElem
-- so the best it can do is to say that val
is a Union{nf_elem, ZZRingElem}
in this function. You can see this in @code_warntype
.
This kind of issue can percolate: Julia may or may not be able to deduce which sign
method is called, and thus may or may not be able to predict the type of sgn
, and then good
, and finally for the return value.
In this case, it seems to be OK as the union splitting code in the Julia compiler can deal with it. But in general I would recommend to avoid re-using a variable for a value of a different type (instead call it val2
or whatever)
good = filter(x -> is_odd(x[2]), collect(factor(val))) | ||
return F(prod([x[1] for x in good], init = sgn)) |
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.
The following might avoid the second allocation. Dunno if it makes things better or not, though...
good = filter(x -> is_odd(x[2]), collect(factor(val))) | |
return F(prod([x[1] for x in good], init = sgn)) | |
good = [x[1] for x in collect(factor(val)) if is_odd(x[2])] | |
return F(prod(good, init = sgn)) |
s = s * p^(e-1) | ||
end | ||
end | ||
return F(c // s) |
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 am confused, isn't c
a vector at this point?
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.
Yes, we want the vector where each entry is divided by s
.
Perhaps it is better to divide val
by s
.
if iseven(e) | ||
s = s * p^e | ||
elseif e > 1 | ||
s = s * p^(e-1) |
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.
s = s * p^(e-1) | |
s *= p^(e-1) |
q2 = ZZ(q)^2 | ||
q2i = ZZ(1) | ||
for i in 1:(m-1) | ||
q2i = q2 * q2i |
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.
q2i = q2 * q2i | |
q2i *= q2 |
res = 1 | ||
for pair in gramdet | ||
if is_odd(pair[2]) | ||
res = res * pair[1] |
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.
res = res * pair[1] | |
res *= pair[1] |
|
||
# Now we know that `chi` belongs to Sym(n) or extends to Sym(n) | ||
gramdet = gram_determinant_specht_module(partition(para)) | ||
res = 1 |
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.
Is this type stable?
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.
Better ZZ(1)
.
(However, for the return value of the function, this does not matter because we call string(res)
.)
How unique does your reduction modulo squares need to be? There is an (unexported) |
@thofma Thanks for the hint.
If the value is in fact in the prime field then I need that the reduction is represented by a squarefree rational integer. |
exp = 0 | ||
while mod(N, q) == 0 | ||
exp = exp + 1 | ||
N = div(N, q) | ||
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.
Actually, I think it might be possible to replace this by
exp = 0 | |
while mod(N, q) == 0 | |
exp = exp + 1 | |
N = div(N, q) | |
end | |
exp, N = remove(N, q) |
while mod(N, p) == 0 | ||
N = div(N, p) | ||
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.
while mod(N, p) == 0 | |
N = div(N, p) | |
end | |
_, N = remove(N, p) |
- add `od_from_order`, `od_from_eigenvalues`, `od_for_specht_module` - add utilities `character_of_entry`, `order_omega_mod_N`, `reduce_mod_squares`
od_from_order
,od_from_eigenvalues
,od_for_specht_module
character_of_entry
,order_omega_mod_N
,reduce_mod_squares