Don't recompute congruence substitutions

[SoongNoonien]

As discussed earlier we shouldn't recompute the substitutions used in the simplification of GenericCyclo. Apparently I've made it slower and increased the allocations when computing scalar products of tables with congruence as in #238 .
Before the patch with SL3.1:
25.557065 seconds (98.68 M allocations: 8.370 GiB, 11.97% gc time)
And after the patch with SL3.1:
36.824178 seconds (111.61 M allocations: 10.982 GiB, 9.44% gc time)

@fingolfin Do you see the problem here?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.82%. Comparing base (833d2c3) to head (ccc0ca0).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #240   +/-   ##
=======================================
  Coverage   95.81%   95.82%           
=======================================
  Files          12       12           
  Lines         836      838    +2     
=======================================
+ Hits          801      803    +2     
  Misses         35       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fingolfin]

Which Julia and OSCAR versions do you use? Did you run @time twice? I am using Julia 1.11.2 and Oscar 1.2.2, and am using this test code:

using GenericCharacterTables
function scalar_test(g)
   for c1 in g
       for c2 in g
           scalar_product(c1, c2)
       end
   end
end
g=generic_character_table("SL3.1")
@time scalar_test(g)
@time scalar_test(g)

With master:

julia> @time scalar_test(g)
  2.845629 seconds (30.61 M allocations: 1.738 GiB, 15.27% gc time, 26.10% compilation time)

julia> @time scalar_test(g)
  2.115675 seconds (29.62 M allocations: 1.689 GiB, 20.44% gc time)

With your PR:

julia> @time scalar_test(g)
  2.889902 seconds (30.64 M allocations: 1.786 GiB, 12.19% gc time, 26.24% compilation time)

julia> @time scalar_test(g)
  2.147462 seconds (29.64 M allocations: 1.736 GiB, 15.47% gc time)

So this is indeed a little bit slower and allocates a bit more, but overall much less than what you have.

[SoongNoonien]

Oh, no... I still had Oscar 1.1.1 installed. Here are the results on my machine with the same versions as on yours.

Current master:

julia> @time scalar_test(g)
 11.932806 seconds (30.61 M allocations: 1.739 GiB, 22.33% gc time, 25.68% compilation time)

julia> @time scalar_test(g)
  7.297629 seconds (29.62 M allocations: 1.689 GiB, 21.61% gc time)

With the PR:

julia> @time scalar_test(g)
 11.660848 seconds (30.63 M allocations: 1.786 GiB, 21.91% gc time, 25.92% compilation time)

julia> @time scalar_test(g)
  7.690424 seconds (29.64 M allocations: 1.736 GiB, 21.38% gc time)

[fingolfin]

As discussed orally, computing this here in advance may be too costly. I would recommend changing the ring to a mutable struct (most of our ring types are anyway, to allow them to store attributes), and then leave the two fields undefined in the constructor.

Then add an accessor function (or two, depending on taste) to get their content, with suitable isdefined(R, :substitute) checks

[fingolfin]

Also, why isn't this

Suggested change

	substitute_inv = (q - congruence[1]) * 1//congruence[2]
	substitute_inv = (q - congruence[1]) / congruence[2]

[fingolfin]

If the fraction was reduced (i.e. numerator and denomiator are coprime) before the substitution, then it still is after, correct? But the // method can't know that and thus may end up trying to reduce? (I don't know if it actually does that, I just wonder).

In any case, you next need numerator and denominator anyway. So perhaps something like this could be used in this loop instead:

  den_g = denominator(g)
  num_g = numerator(g)
  if R.congruence === nothing
    den_g = evaluate(den_g, [1], [substitute])
    num_g = evaluate(num_g, [1], [substitute])
  end
  a, r = divrem(num_g, den_g)
  push!(L, (c, den_g, r, a))

This would arguably also make the code a little bit nicer?

[SoongNoonien]

If the fraction was reduced (i.e. numerator and denomiator are coprime) before the substitution, then it still is after, correct?

No, not necessarily. This is kind of the whole point of this substitution. Take for example the fraction (q+1)//2. If q is now congruent to 1 modulo 2 then R.substitute is equal to 2*q+1. After evaluation the fraction is (2*q+2)//2 which is not reduced anymore. The idea is then that the fraction gets reduced to q+1 which has a normal form of 0. This detects that exp(2*pi*i*(q+1)//2) is equal to 1 for all q congruent to 1 modulo 2.

[fingolfin]

ah of course, gotcha.

So please add a comment explaining just that? :-)

[fingolfin]

Style / taste question, but: I like to avoid nested control flow, esp. multi-level, to improve code clarity. So here I'd do

Suggested change

	for (g, c) in f
	for (g, c) in f
	is_zero(c) && continue

and then drop the if !iszero(c) and thereby reduce the indention level inside the loop