Change _zero_algebra -> zero_algebra

[fingolfin]

The function _zero_algebra was renamed to zero_algebra some time ago, but not all places calling it were updated.

I found this because it caused a bunch of invalidations in code found while investigating #746 (indirectly, of course, as any actual call to _zero_algebra could not have worked).

[thofma]

Makes inference worse in some "unrelated" place, thus making the tests fail. (I have seen this happening before. I still don't know why.)

[lgoettgens]

Running these failing tests in the REPL does not fail, so this kind of weird to debug.

[fingolfin]

Here's a theory why: code which calls _zero_algebra by definition cannot work (because there is no _zero_algebra). LLVM may then classify this code as having UB (undefined behavior) and that enables many "optimizations" (basically you can replace calls to such code with anything). That in turn may make it possible to "prove" that certain functions are type stable, when they really are not.

If this guess is right, then anything that now fails inference should be checked as to whether it directly or indirectly leads to calls to _zero_algebra

[fingolfin]

I can reproduce this in the REPL:

julia> A = matrix_algebra(GF(2), 2)
Matrix algebra of dimension 4 over Prime field of characteristic 2

julia> @inferred Hecke._unit_group_generators(A)
ERROR: return type Vector{MatAlgebraElem{FqFieldElem, FqMatrix}} does not match inferred return type Any

This goes away if I change the last line of Hecke._unit_group_generators from return map(A, res) to return res or return A, res.

But @code_warntype reports nothing suspicious and indeed even displays the correct and concrete return type. Moreover, if I use the modified version with return res, and then do @inferred map(A,res) on the top level, there are no complaints...

Could this be a bug in @inferred then?!

[thofma]

Yeah, more (random) inference errors.

[fingolfin]

Fascinating. With this patch,

    E = elliptic_curve([1, 2, 3, 4, 5])
    a, b = @inferred CPS_height_bounds(E)

fails because inside CPS_height_bounds the return type of CPS_dvev_real can no longer be inferred.

OK, I can reduce this to

    E = elliptic_curve([1, 2, 3, 4, 5])
    @inferred Hecke.CPS_dvev_real(E, PosInf(), 110)

There, it fails to infer the full return type for Hecke._roots. With this PR I see

│   %156 = Core.kwcall(%155, Hecke._roots, f, v)::Tuple{Vector{AcbFieldElem}, Any, Vector{AcbFieldElem}}

without the PR I see

│   %156 = Core.kwcall(%155, Hecke._roots, f, v)::Tuple{Vector{AcbFieldElem}, Vector{ArbFieldElem}, Vector{AcbFieldElem}}

Narrowing it down:

Kx, x = polynomial_ring(QQ, "x")
b2, b4, b6, b8 = (9, 11, 29, 35)
f = 4*x^3 + b2*x^2 + 2*b4*x + b6
@inferred Hecke._roots(f, PosInf(), prec = 110)

Will dig more later.

[lgoettgens]

I found a very simple example that differs between master and this branch:

julia> y = [AcbField(64)(1)]
1-element Vector{AcbFieldElem}:
 1.0000000000000000000

julia> @code_warntype real.(y)
MethodInstance for (::var"##dotfunction#234#8")(::Vector{AcbFieldElem})
  from (::var"##dotfunction#234#8")(x1) @ Main none:0
Arguments
  #self#::Core.Const(var"##dotfunction#234#8"())
  x1::Vector{AcbFieldElem}
Body::Any
1 ─ %1 = Base.broadcasted(Main.real, x1)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(real), Tuple{Vector{AcbFieldElem}}}
│   %2 = Base.materialize(%1)::Any
└──      return %2

on this branch, while on master:

julia> @code_warntype real.(y)
MethodInstance for (::var"##dotfunction#225#1")(::Vector{AcbFieldElem})
  from (::var"##dotfunction#225#1")(x1) @ Main none:0
Arguments
  #self#::Core.Const(var"##dotfunction#225#1"())
  x1::Vector{AcbFieldElem}
Body::Vector{ArbFieldElem}
1 ─ %1 = Base.broadcasted(Main.real, x1)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(real), Tuple{Vector{AcbFieldElem}}}
│   %2 = Base.materialize(%1)::Vector{ArbFieldElem}
└──      return %2

This then makes inference fail in

Hecke.jl/src/Misc/Poly.jl

Line 731 in b3c2264

rl_rts = real.(filter(isreal, all_rts))

leading to the problems observed by @fingolfin

[fingolfin]

@lgoettgens thanks. Unfortunately that seems to be a dead end:

code_warntype(Base.materialize, (Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, typeof(real), Tuple{Vector{AcbFieldElem}}}, ))

prints perfectly inferred information in both branches.

I tried using JET but @report_opt broadcast(real, y) reports no issues (note that @code_warntype broadcast(real, y) shows the same behavior as your @code_warntype real.(y)

And according to @descend broadcast(real, y) everything is fine, too.

Aaargh!

[fingolfin]

Hah, I just tried with Julia 1.11.0-rc2 and there it works (both Lars example and also the elliptic curve example).

But it's not as if all was great: with that Julia, I get an inference failure with

g = @inferred supersingular_polynomial(2)

which works fine in Julia 1.10 (both with my branch and with master).

Seems we just can't have nice things sigh

[codecov]

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 75.79%. Comparing base (23f1dcc) to head (0620101).
Report is 24 commits behind head on master.

Files	Patch %	Lines
src/AlgAss/AlgAss.jl	0.00%	6 Missing ⚠️
src/GenOrd/Ideal.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1504      +/-   ##
==========================================
+ Coverage   75.69%   75.79%   +0.10%     
==========================================
  Files         357      358       +1     
  Lines      113691   113692       +1     
==========================================
+ Hits        86059    86174     +115     
+ Misses      27632    27518     -114     

Files	Coverage Δ
src/AlgAssAbsOrd/LocallyFreeClassGroup.jl	92.47% <ø> (+0.14%)	⬆️
src/Misc/Poly.jl	69.42% <100.00%> (ø)
src/GenOrd/Ideal.jl	77.50% <0.00%> (ø)
src/AlgAss/AlgAss.jl	87.21% <0.00%> (ø)

... and 55 files with indirect coverage changes