Update to latest AA, Nemo #1294

fingolfin · 2023-11-23T21:38:39Z

CyclotomicRealSubfield -> cyclotomic_real_subfield
isconstant -> is_constant
switch some uses of vcat to `reduce(vcat, ...)

fingolfin · 2023-11-24T13:56:05Z

The invalidation count increases from 610 to 612. This is expected: the new "ring with variable names" constructor code" introduced this raises in AA, and we decided it was not important enough to hold back merging. (I plan to revisit this and other invalidations at another time).

The errors with Julia 1.6 are all (?) about failures to infer return types -- they are inferred as Any. E.g.:

Misc: Error During Test at /home/runner/work/Hecke.jl/Hecke.jl/test/QuadForm/Lattices.jl:167
  Got exception outside of a @test
  return type nf_elem does not match inferred return type Any
  Stacktrace:
    [1] error(s::String)
      @ Base ./error.jl:33
    [2] macro expansion
      @ ~/work/Hecke.jl/Hecke.jl/test/QuadForm/Lattices.jl:173 [inlined]

What I don't understand is why these only happen in Julia 1.6 but not 1.9, and how the changes in this PR might be causing them... Well perhaps by making some return types harder to infer and 1.9 is just better at doing it... ? But that's still rather vague. I guess it'll require looking at some of these cases more closely?

Finally the error with Julia nightly I don't understand:

Picard group and unit group of non maximal orders: Error During Test at /home/runner/work/Hecke.jl/Hecke.jl/test/NfOrd/PicardGroup.jl:61
  Got exception outside of a @test
  conversion to pointer not defined for Vector{Ptr{Nothing}}
  Stacktrace:
    [1] error(s::String)
      @ Base ./error.jl:35
    [2] unsafe_convert(::Type{Ptr{Ptr{Nothing}}}, a::Vector{Ptr{Nothing}})
      @ Base ./pointer.jl:67
    [3] unsafe_convert(::Type{Ptr{Nothing}}, a::Vector{Ptr{Nothing}})
      @ Base ./pointer.jl:66
    [4] Hecke.VeryBad(n::UInt64, ninv::UInt64, norm::UInt64)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/Misc/FiniteField.jl:272
    [5] Hecke.FrobeniusCtx(K::fqPolyRepField, i::Int64)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/Misc/FiniteField.jl:300
    [6] Hecke.FrobeniusCtx(K::fqPolyRepField)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/Misc/FiniteField.jl:299
    [7] iterate
      @ Base ./generator.jl:48 [inlined]
    [8] _collect
      @ Base ./array.jl:793 [inlined]
    [9] collect_similar(cont::Vector{fqPolyRepField}, itr::Base.Generator{Vector{fqPolyRepField}, Type{Hecke.FrobeniusCtx}})
      @ Base ./array.jl:702
   [10] map
      @ ./abstractarray.jl:3307 [inlined]
   [11] modular_proj_vec(A::FacElem{nf_elem, AnticNumberField}, me::Hecke.modular_env)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/Misc/CRT.jl:687
   [12] modular_proj(A::FacElem{nf_elem, AnticNumberField}, me::Hecke.modular_env)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/Misc/CRT.jl:649
   [13] evaluate_mod(a::FacElem{nf_elem, AnticNumberField}, B::Hecke.NfAbsOrdFracIdl{AnticNumberField, nf_elem})
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumField/NfAbs/CompactRepresentation.jl:337
   [14] compact_presentation(a::FacElem{nf_elem, AnticNumberField}, nn::Int64; decom::Dict{NfOrdIdl, ZZRingElem}, arb_prec::Int64, short_prec::Int64)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumField/NfAbs/CompactRepresentation.jl:15
   [15] macro expansion
      @ ~/work/Hecke.jl/Hecke.jl/src/Hecke.jl:500 [inlined]
   [16] _ispower(a::FacElem{nf_elem, AnticNumberField}, n::Int64; with_roots_unity::Bool, decom::Dict{NfOrdIdl, ZZRingElem}, trager::Bool)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumField/NfAbs/CompactRepresentation.jl:435
   [17] is_power(a::FacElem{nf_elem, AnticNumberField}, n::Int64; with_roots_unity::Bool, decom::Dict{NfOrdIdl, ZZRingElem}, trager::Bool, easy::Bool)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumField/NfAbs/CompactRepresentation.jl:427
   [18] macro expansion
      @ ~/work/Hecke.jl/Hecke.jl/src/Hecke.jl:500 [inlined]
   [19] saturate!(d::Hecke.ClassGrpCtx{SMat{ZZRingElem, Hecke.ZZRingElem_Array_Mod.ZZRingElem_Array}}, U::Hecke.UnitGrpCtx{FacElem{nf_elem, AnticNumberField}}, n::Int64, stable::Float64; use_orbit::Bool, easy_root::Bool, use_LLL::Bool)
      @ Hecke.RelSaturate ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/Clgp/Saturate.jl:449
   [20] saturate!(d::Hecke.ClassGrpCtx{SMat{ZZRingElem, Hecke.ZZRingElem_Array_Mod.ZZRingElem_Array}}, U::Hecke.UnitGrpCtx{FacElem{nf_elem, AnticNumberField}}, n::Int64, stable::Float64)
      @ Hecke.RelSaturate ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/Clgp/Saturate.jl:411
   [21] _class_unit_group(O::NfOrd; saturate_at_2::Bool, bound::Int64, method::Int64, large::Int64, redo::Bool, unit_method::Int64, use_aut::Bool, GRH::Bool)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/Clgp.jl:308
   [22] class_group(O::NfOrd; bound::Int64, method::Int64, redo::Bool, unit_method::Int64, large::Int64, use_aut::Bool, GRH::Bool, do_lll::Bool, saturate_at_2::Bool)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/Clgp.jl:435
   [23] class_group(O::NfOrd)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/Clgp.jl:423
   [24] _picard_group(O::NfOrd)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/PicardGroup.jl:208
   [25] (::Hecke.var"#2725#2726"{NfOrd})()
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/PicardGroup.jl:21
   [26] get!(default::Hecke.var"#2725#2726"{NfOrd}, h::Dict{Symbol, Any}, key::Symbol)
      @ Base ./dict.jl:491
   [27] get_attribute!(f::Function, G::NfOrd, attr::Symbol)
      @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/49QmB/src/Attributes.jl:230
   [28] picard_group(O::NfOrd)
      @ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/PicardGroup.jl:16
   [29] macro expansion
      @ ~/work/Hecke.jl/Hecke.jl/test/NfOrd/PicardGroup.jl:110 [inlined]
   [30] macro expansion
      @ /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:1598 [inlined]
   [31] top-level scope
      @ ~/work/Hecke.jl/Hecke.jl/test/NfOrd/PicardGroup.jl:62
   [32] include(fname::String)
      @ Main ./sysimg.jl:38
   [33] macro expansion
      @ ~/work/Hecke.jl/Hecke.jl/test/NfOrd.jl:14 [inlined]
   [34] macro expansion
      @ /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:1598 [inlined]
   [35] top-level scope
      @ ~/work/Hecke.jl/Hecke.jl/test/NfOrd.jl:2
   [36] include(fname::String)
      @ Main ./sysimg.jl:38
   [37] top-level scope
      @ ~/work/Hecke.jl/Hecke.jl/test/runtests.jl:280
   [38] include(fname::String)
      @ Main ./sysimg.jl:38
   [39] top-level scope
      @ none:6
   [40] eval
      @ Core ./boot.jl:426 [inlined]
   [41] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:291
   [42] _start()
      @ Base ./client.jl:525

thofma · 2023-11-24T14:07:22Z

The failure on nightly is "expected", see JuliaLang/julia#51996.

The error on 1.6 is not nice, but it seems that the Nemo/AA changes led to some less precise inference.

fingolfin · 2023-11-28T13:18:45Z

I tracked down the Julia 1.6 type instability to one concrete example where I finally see some relation to the AA updates:

Qx, x = FlintQQ["x"]
f = x^2 + 12x - 92
K, a = number_field(f, "a")
Ky, y = K["y"]

L, b = number_field(y^2 + y + 1, "b")

@code_warntype polynomial_ring(L, "y", cached = false)  # <- type unstable in Julia 1.6, but stable in 1.9

This in turn is due to this code in AA:

@varnames_interface polynomial_ring(R::Ring, s)

# With `Ring <: NCRing`, we need to resolve ambiguities of `polynomial_ring(::Ring, s...)`
polynomial_ring(R::Ring, s::Symbol; kv...) = invoke(polynomial_ring, Tuple{NCRing, Symbol}, R, s; kv...)
polynomial_ring(R::Ring, s::Union{AbstractString, Char}; kv...) = polynomial_ring(R, Symbol(s); kv...)

I suspect the invoke is the problem, and quite frankly I don't like this code (I failed to notice it when reviewing the polynomial_ring PR). I'll try to understand what goes on exactly and hopefully can make a fix for AA this week.

thofma · 2023-11-28T13:33:31Z

Hm, yeah, I am not happy about the invoke either. Did not know we had to resort to such things. Can't we do some auxiliary _polynomial_ring magic to circumvent this?

codecov · 2023-12-01T17:14:21Z

Codecov Report

Merging #1294 (23930bc) into master (791e87a) will increase coverage by 69.03%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1294       +/-   ##
===========================================
+ Coverage    5.53%   74.56%   +69.03%     
===========================================
  Files         346      346               
  Lines      111652   111795      +143     
===========================================
+ Hits         6177    83362    +77185     
+ Misses     105475    28433    -77042

Files	Coverage Δ
src/LargeField/misc2.jl	`0.00% <ø> (ø)`
src/Misc/Poly.jl	`62.11% <100.00%> (+50.91%)`	⬆️
src/NumField/NfAbs/Cyclotomic.jl	`100.00% <100.00%> (+100.00%)`	⬆️
src/NumField/NfRel/NfRel.jl	`78.55% <100.00%> (+78.55%)`	⬆️

... and 302 files with indirect coverage changes

fingolfin · 2023-12-01T17:20:31Z

With the new AA release, tests pass in 1.6. However, weirdly there is a failure with Julia 1.9 now -- though it passes on my computer, and I don't see a relation to the changes in this PR right now. Perhaps re-run the tests, @thofma ?

LocalFundamentalClass Serre: Test Failed at /home/runner/work/Hecke.jl/Hecke.jl/test/LocalField/neq.jl:57
  Expression: iszero(a) || valuation(a) > 20

Stacktrace:
 [1] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.4/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
 [2] macro expansion
   @ ~/work/Hecke.jl/Hecke.jl/test/LocalField/neq.jl:57 [inlined]
 [3] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.4/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
 [4] top-level scope
   @ ~/work/Hecke.jl/Hecke.jl/test/LocalField/neq.jl:45
LocalFundamentalClass Serre: Test Failed at /home/runner/work/Hecke.jl/Hecke.jl/test/LocalField/neq.jl:57
  Expression: iszero(a) || valuation(a) > 20

Stacktrace:
 [1] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.4/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
 [2] macro expansion
   @ ~/work/Hecke.jl/Hecke.jl/test/LocalField/neq.jl:57 [inlined]
 [3] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.4/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
 [4] top-level scope
   @ ~/work/Hecke.jl/Hecke.jl/test/LocalField/neq.jl:45

thofma · 2023-12-01T17:30:15Z

Have not seen this in a while. I have restarted the test.

thofma · 2023-12-01T23:00:06Z

I will merge once I landed #1306.

fingolfin · 2023-12-05T23:41:23Z

Finally passing...

thofma · 2023-12-06T07:47:59Z

I am a bit reluctant to merge this before Nemocas/AbstractAlgebra.jl#1518 is fixed.

lgoettgens · 2023-12-07T08:18:46Z

test/Aqua.jl

@@ -4,6 +4,6 @@ using Aqua
  Aqua.test_all(
    Hecke;
    ambiguities=false,      # TODO: fix ambiguities
-    piracies=false          # TODO: fix piracy
+    piracies=true          # TODO: fix piracy


Suggested change

piracies=true # TODO: fix piracy

piracies=false # TODO: fix piracy

This change shouldn't be in this PR.

Argh, indeed, that slipped in while I made another experiment. Thank you

- CyclotomicRealSubfield -> cyclotomic_real_subfield - isconstant -> is_constant

fingolfin · 2023-12-07T10:35:14Z

OK I think this is ready now, @thofma ?

fingolfin force-pushed the mh/upd branch 3 times, most recently from ff9f7e1 to b6a029e Compare November 24, 2023 06:47

fingolfin mentioned this pull request Nov 24, 2023

Replace some vcat and hcat calls by reduce #1295

Merged

fingolfin force-pushed the mh/upd branch from b6a029e to 265bf54 Compare December 1, 2023 14:37

fingolfin force-pushed the mh/upd branch 2 times, most recently from 737b8cc to 01788b3 Compare December 5, 2023 10:17

thofma mentioned this pull request Dec 5, 2023

Fix some recent matrix methods Nemocas/AbstractAlgebra.jl#1516

Merged

fingolfin force-pushed the mh/upd branch from 01788b3 to 137122e Compare December 5, 2023 21:45

fingolfin force-pushed the mh/upd branch from 137122e to bcd0901 Compare December 6, 2023 21:36

lgoettgens reviewed Dec 7, 2023

View reviewed changes

Update to latest AA, Nemo

23930bc

- CyclotomicRealSubfield -> cyclotomic_real_subfield - isconstant -> is_constant

fingolfin force-pushed the mh/upd branch from bcd0901 to 23930bc Compare December 7, 2023 08:22

thofma merged commit 6e03905 into thofma:master Dec 7, 2023
17 of 18 checks passed

fingolfin deleted the mh/upd branch December 7, 2023 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to latest AA, Nemo #1294

Update to latest AA, Nemo #1294

fingolfin commented Nov 23, 2023 •

edited

Loading

fingolfin commented Nov 24, 2023

thofma commented Nov 24, 2023

fingolfin commented Nov 28, 2023

thofma commented Nov 28, 2023

codecov bot commented Dec 1, 2023 •

edited

Loading

fingolfin commented Dec 1, 2023

thofma commented Dec 1, 2023

thofma commented Dec 1, 2023

fingolfin commented Dec 5, 2023

thofma commented Dec 6, 2023

lgoettgens Dec 7, 2023

fingolfin Dec 7, 2023

fingolfin commented Dec 7, 2023

	piracies=true # TODO: fix piracy
	piracies=false # TODO: fix piracy

Update to latest AA, Nemo #1294

Update to latest AA, Nemo #1294

Conversation

fingolfin commented Nov 23, 2023 • edited Loading

fingolfin commented Nov 24, 2023

thofma commented Nov 24, 2023

fingolfin commented Nov 28, 2023

thofma commented Nov 28, 2023

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

fingolfin commented Dec 1, 2023

thofma commented Dec 1, 2023

thofma commented Dec 1, 2023

fingolfin commented Dec 5, 2023

thofma commented Dec 6, 2023

lgoettgens Dec 7, 2023

Choose a reason for hiding this comment

fingolfin Dec 7, 2023

Choose a reason for hiding this comment

fingolfin commented Dec 7, 2023

fingolfin commented Nov 23, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading