some methods for computing orth. discriminants #2748

ThomasBreuer · 2023-08-30T14:43:44Z

add od_from_order, od_from_eigenvalues, od_for_specht_module
add utilities character_of_entry, order_omega_mod_N, reduce_mod_squares

- add `od_from_order`, `od_from_eigenvalues`, `od_for_specht_module - add utilities `character_of_entry`, `order_omega_mod_N`, `reduce_mod_squares`

codecov · 2023-08-30T23:41:05Z

Codecov Report

Merging #2748 (2f6f7c0) into master (f562757) will increase coverage by 0.21%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

@@            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

Files Changed	Coverage
...ogonalDiscriminants/src/OrthogonalDiscriminants.jl	`ø`
experimental/OrthogonalDiscriminants/src/data.jl	`100.00%`
...imental/OrthogonalDiscriminants/src/theoretical.jl	`100.00%`
experimental/OrthogonalDiscriminants/src/utils.jl	`100.00%`
...mental/OrthogonalDiscriminants/test/theoretical.jl	`100.00%`
experimental/OrthogonalDiscriminants/test/utils.jl	`100.00%`

fingolfin

Fine by me, but some questions and suggestions -- but all optional, feel free to ignore some or all.

fingolfin · 2023-08-31T21:23:03Z

experimental/OrthogonalDiscriminants/src/utils.jl

+```
+"""
+function reduce_mod_squares(F::AnticNumberField, val::nf_elem)
+  @req parent(val) === F "val must have parent F"


Why is F even an argument when the only place using it is this check?

Yes.
I still have to get used to the fact that the elements carry the relevant context.

fingolfin · 2023-08-31T21:23:19Z

experimental/OrthogonalDiscriminants/src/utils.jl

+  is_zero(val) && return val
+  d = denominator(val)
+  if ! isone(d)
+    val = val * d^2


Suggested change

val = val * d^2

val *= d^2

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.)

From my POV either is fine, this is a matter of taste. Pick whichever you prefer :-)

fingolfin · 2023-08-31T21:24:47Z

experimental/OrthogonalDiscriminants/src/utils.jl

+    val = val * d^2
+  end
+  if is_integer(val)
+    val = ZZ(val)


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.

I do not understand this comment. In the end, an element of F is returned.

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)

fingolfin · 2023-08-31T21:28:56Z

experimental/OrthogonalDiscriminants/src/utils.jl

+    good = filter(x -> is_odd(x[2]), collect(factor(val)))
+    return F(prod([x[1] for x in good], init = sgn))


The following might avoid the second allocation. Dunno if it makes things better or not, though...

Suggested change

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))

fingolfin · 2023-08-31T21:30:12Z

experimental/OrthogonalDiscriminants/src/utils.jl

+      s = s * p^(e-1)
+    end
+  end
+  return F(c // s)


I am confused, isn't c a vector at this point?

Yes, we want the vector where each entry is divided by s.
Perhaps it is better to divide val by s.

fingolfin · 2023-08-31T21:30:36Z

experimental/OrthogonalDiscriminants/src/utils.jl

+    if iseven(e)
+      s = s * p^e
+    elseif e > 1
+      s = s * p^(e-1)


Suggested change

s = s * p^(e-1)

s *= p^(e-1)

fingolfin · 2023-08-31T21:32:15Z

experimental/OrthogonalDiscriminants/src/utils.jl

+  q2 = ZZ(q)^2
+  q2i = ZZ(1)
+  for i in  1:(m-1)
+    q2i = q2 * q2i


Suggested change

q2i = q2 * q2i

q2i *= q2

experimental/OrthogonalDiscriminants/src/utils.jl

fingolfin · 2023-08-31T21:32:56Z

experimental/OrthogonalDiscriminants/src/theoretical.jl

+  res = 1
+  for pair in gramdet
+    if is_odd(pair[2])
+      res = res * pair[1]


Suggested change

res = res * pair[1]

res *= pair[1]

fingolfin · 2023-08-31T21:33:27Z

experimental/OrthogonalDiscriminants/src/theoretical.jl

+
+  # Now we know that `chi` belongs to Sym(n) or extends to Sym(n)
+  gramdet = gram_determinant_specht_module(partition(para))
+  res = 1


Is this type stable?

Better ZZ(1).
(However, for the return value of the function, this does not matter because we call string(res).)

thofma · 2023-09-01T12:18:08Z

How unique does your reduction modulo squares need to be? There is an (unexported) Hecke.reduce_mod_powers(::nf_elem, ::Int), which also tries to reduce something modulo powers.

ThomasBreuer · 2023-09-01T12:25:35Z

@thofma Thanks for the hint.

How unique does your reduction modulo squares need to be?

If the value is in fact in the prime field then I need that the reduction is represented by a squarefree rational integer.
Otherwise I cannot expect uniqueness, and what Hecke.reduce_mod_powers promises seems to be useful.

fingolfin · 2023-09-05T08:49:45Z

experimental/OrthogonalDiscriminants/src/utils.jl

+  exp = 0
+  while mod(N, q) == 0
+    exp = exp + 1
+    N = div(N, q)
+  end


Actually, I think it might be possible to replace this by

Suggested change

exp = 0

while mod(N, q) == 0

exp = exp + 1

N = div(N, q)

end

exp, N = remove(N, q)

experimental/OrthogonalDiscriminants/src/utils.jl

fingolfin · 2023-09-05T08:53:42Z

experimental/OrthogonalDiscriminants/src/utils.jl

+    while mod(N, p) == 0
+      N = div(N, p)
+    end


Suggested change

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`

ThomasBreuer added 2 commits August 30, 2023 16:40

some methods for computing orth. discriminants

fc3d1e6

- add `od_from_order`, `od_from_eigenvalues`, `od_for_specht_module - add utilities `character_of_entry`, `order_omega_mod_N`, `reduce_mod_squares`

fixed a doctest

2f6f7c0

fingolfin approved these changes Aug 31, 2023

View reviewed changes

fingolfin reviewed Sep 5, 2023

View reviewed changes

experimental/OrthogonalDiscriminants/src/utils.jl Show resolved Hide resolved

fingolfin reviewed Sep 5, 2023

View reviewed changes

ThomasBreuer mentioned this pull request Sep 5, 2023

Hecke.reduce_mod_powers thofma/Hecke.jl#1202

Closed

address comments

ccbf846

fingolfin enabled auto-merge (squash) September 8, 2023 21:06

fingolfin merged commit 79874fd into oscar-system:master Sep 8, 2023
8 of 12 checks passed

fieker pushed a commit that referenced this pull request Sep 29, 2023

some methods for computing orth. discriminants (#2748)

ee2da7a

- add `od_from_order`, `od_from_eigenvalues`, `od_for_specht_module` - add utilities `character_of_entry`, `order_omega_mod_N`, `reduce_mod_squares`

ThomasBreuer deleted the TB_OD_first_methods branch October 10, 2023 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

some methods for computing orth. discriminants #2748

some methods for computing orth. discriminants #2748

ThomasBreuer commented Aug 30, 2023

codecov bot commented Aug 30, 2023

fingolfin left a comment

fingolfin Aug 31, 2023

ThomasBreuer Sep 1, 2023

fingolfin Aug 31, 2023

ThomasBreuer Sep 1, 2023

fingolfin Sep 1, 2023

fingolfin Aug 31, 2023

ThomasBreuer Sep 1, 2023

fingolfin Sep 5, 2023

fingolfin Aug 31, 2023

fingolfin Aug 31, 2023

ThomasBreuer Sep 1, 2023

fingolfin Aug 31, 2023

fingolfin Aug 31, 2023

fingolfin Aug 31, 2023

fingolfin Aug 31, 2023

ThomasBreuer Sep 1, 2023

thofma commented Sep 1, 2023

ThomasBreuer commented Sep 1, 2023

fingolfin Sep 5, 2023

fingolfin Sep 5, 2023

		good = filter(x -> is_odd(x[2]), collect(factor(val)))
		return F(prod([x[1] for x in good], init = sgn))

some methods for computing orth. discriminants #2748

some methods for computing orth. discriminants #2748

Conversation

ThomasBreuer commented Aug 30, 2023

codecov bot commented Aug 30, 2023

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thofma commented Sep 1, 2023

ThomasBreuer commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment