Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix vinberg booktests #3939

Merged
merged 5 commits into from
Jul 24, 2024
Merged

Fix vinberg booktests #3939

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
942bb3b
fix booktests for vinberg_3.jlcon and move it to vinberg_2.jlcon
simonbrandhorst Jul 15, 2024
0870a49
remove vinberg from ordered_examples.json
simonbrandhorst Jul 16, 2024
a913f80
fix tests
simonbrandhorst Jul 17, 2024
8d48266
suppress printing of unstable outputs
simonbrandhorst Jul 23, 2024
5f00b70
booktest: explicitly destroy mongodb db and client (#3963)
benlorenz Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions experimental/Schemes/src/elliptic_surface.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1350,6 +1350,7 @@ function horizontal_decomposition(X::EllipticSurface, F::Vector{QQFieldElem})
E = generic_fiber(X)
basisNS, tors, NS = algebraic_lattice(X)
V = ambient_space(NS)
@req F in algebraic_lattice(X)[3] "not in the algebraic lattice"
@req inner_product(V, F, F)==0 "not an isotropic divisor"
@req euler_characteristic(X) == 2 "not a K3 surface"
# how to give an ample divisor automagically in general?
Expand Down
3 changes: 1 addition & 2 deletions test/book/ordered_examples.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,7 @@
],
"specialized/brandhorst-zach-fibration-hopping": [
"vinberg_1.jlcon",
"vinberg_2.jlcon",
"vinberg_3.jlcon"
"vinberg_2.jlcon"
],
"specialized/breuer-nebe-parker-orthogonal-discriminants": [
"expl_order.jlcon",
Expand Down
106 changes: 85 additions & 21 deletions test/book/specialized/brandhorst-zach-fibration-hopping/vinberg_2.jlcon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,26 @@ with default covering
6: [(z//y), (x//y), s]

julia> piS = weierstrass_contraction(Y1)
Composite morphism of[...]
Composite morphism of
Hom: elliptic surface with generic fiber -x^3 + y^2 - t^7 + 2*t^6 - t^5 -> scheme over QQ covered with 44 patches
Hom: scheme over QQ covered with 44 patches -> scheme over QQ covered with 40 patches
Hom: scheme over QQ covered with 40 patches -> scheme over QQ covered with 38 patches
Hom: scheme over QQ covered with 38 patches -> scheme over QQ covered with 36 patches
Hom: scheme over QQ covered with 36 patches -> scheme over QQ covered with 32 patches
Hom: scheme over QQ covered with 32 patches -> scheme over QQ covered with 30 patches
Hom: scheme over QQ covered with 30 patches -> scheme over QQ covered with 28 patches
Hom: scheme over QQ covered with 28 patches -> scheme over QQ covered with 26 patches
Hom: scheme over QQ covered with 26 patches -> scheme over QQ covered with 24 patches
Hom: scheme over QQ covered with 24 patches -> scheme over QQ covered with 22 patches
Hom: scheme over QQ covered with 22 patches -> scheme over QQ covered with 20 patches
Hom: scheme over QQ covered with 20 patches -> scheme over QQ covered with 18 patches
Hom: scheme over QQ covered with 18 patches -> scheme over QQ covered with 16 patches
Hom: scheme over QQ covered with 16 patches -> scheme over QQ covered with 14 patches
Hom: scheme over QQ covered with 14 patches -> scheme over QQ covered with 12 patches
Hom: scheme over QQ covered with 12 patches -> scheme over QQ covered with 10 patches
Hom: scheme over QQ covered with 10 patches -> scheme over QQ covered with 6 patches



julia> basisNSY1, gramTriv = trivial_lattice(Y1);

Expand Down Expand Up @@ -88,7 +107,7 @@ julia> basisNSY1
julia> basisNSY1[1]
Effective weil divisor Fiber over (2, 1)
on elliptic surface with generic fiber -x^3 + y^2 - t^7 + 2*t^6 - t^5
with coefficients in integer Ring
with coefficients in integer ring
given as the formal sum of
1 * sheaf of ideals

Expand All @@ -100,19 +119,19 @@ julia> @assert gram_matrix(NSY1) == gram_matrix(NS)[I,I]
julia> Oscar.horizontal_decomposition(Y1, fibers[2][I])[2]
Effective weil divisor
on elliptic surface with generic fiber -x^3 + y^2 - t^7 + 2*t^6 - t^5
with coefficients in integer Ring
with coefficients in integer ring
given as the formal sum of
2 * component E8_3 of fiber over (0, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the order here keeps changing, that suggests a missing hash method for somewhere; well, most likely for the keys (resp. the type of the keys) in some dictionary. Is there a dict being used here? What is the type of the keys?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of the dictionary is
IdDict{Oscar.AbsIdealSheaf, ZZRingElem}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so AbsIdealSheaf is an abstract type, but all its subtypes seem to end with IdealSheaf. Alas git grep hash.*IdealSheaf reveals zero hits, i.e. no hashing methods for any of those types. At the same time, there is an equality method in experimental/Schemes/src/IdealSheaves.jl:466:

function ==(I::AbsIdealSheaf, J::AbsIdealSheaf)
  I === J && return true
  X = space(I)
  X == space(J) || return false
  for U in basic_patches(default_covering(X))
    is_subset(I(U), J(U)) && is_subset(J(U), I(U)) || return false
  end
  return true
end

But the rules are clear: when you define a custom == method you have to ensure hash still satisfies the implication if a==b then hash(a)==hash(b) which usually means a custom hash method has to be provided.

The absolute minimal "always correct" hash function would be this:

Base.hash(x::AbsIdealSheaf, h::UInt) = h

So you could try adding this, perhaps with a comment # TODO: replace by something better

The drawback of such a hash function is that it makes lookups of a key in a Dict slower. The upside is that it also makes it less wrong, which I think is overall preferable in our business... ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, but you are actually storing things in an IdDict -- so in that case the hash method does not matter at all.

But also: you have zero control over the order, because now instead of hash it uses objectid which essentially is the address of the object, and that of course can change when you re-run the program.

In that case the only fix I can think of is to modify your printing code to "sort" the keys in some way, based on some parameters -- doesn't have to be perfect (i.e. not a total order), just good enough to stabilize the output for this example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example, perhaps one could "cheaply sort" the entries so that those with a "component" in the output string come (for example) first; and then those in turn are sorted on these strings E8_4 etc.; and when there is a tie, sort next by the points they are a fiber over (i.e. (0,1) and so on?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Or switch to a Dict after adding a suitable hash function -- but perhaps your == method is too slow for that?)

BTWH hash(h) = h will still not give fully stable output, but assuming your algorithm is deterministic, or at least runs with a fixed RNG seed (as is the case here) the order should be stable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you guessed we chose IdDict because == is prohibitively expensive

2 * component E8_4 of fiber over (0, 1)
2 * component E8_6 of fiber over (0, 1)
1 * component E8_0 of fiber over (1, 0)
2 * component E8_7 of fiber over (0, 1)
1 * component E8_8 of fiber over (0, 1)
2 * section: (0 : 1 : 0)
1 * component A2_0 of fiber over (1, 1)
1 * component E8_0 of fiber over (1, 0)
2 * component E8_4 of fiber over (0, 1)
2 * component E8_3 of fiber over (0, 1)
1 * component E8_2 of fiber over (0, 1)
2 * component E8_0 of fiber over (0, 1)
2 * component E8_6 of fiber over (0, 1)
2 * component E8_5 of fiber over (0, 1)
1 * component E8_8 of fiber over (0, 1)
2 * component E8_0 of fiber over (0, 1)

julia> representative(elliptic_parameter(Y1, fibers[2][I]))
(x//z)//(t^3 - t^2)
Expand All @@ -122,22 +141,20 @@ julia> g, phi1 = two_neighbor_step(Y1, fibers[2][I]); g

julia> kt2 = base_ring(parent(g)); P = kt2.([0,0]);

julia> Y2, phi2 = elliptic_surface(g, P); Y2
julia> Y2, phi2 = elliptic_surface(g, P; minimize=false); Y2
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure in which Oscar versions this works. It is a recent addition. in 5dcfffb

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should work in 1.1.1, but not in 1.1.0.

Elliptic surface
over rational field
with generic fiber
-x^3 + t^3*x^2 - t^3*x + y^2
and relatively minimal model
scheme over QQ covered with 47 patches

julia> E2 = generic_fiber(Y2); t2 = gen(kt2);
julia> E2 = generic_fiber(Y2); tt = gen(kt2);

julia> P2 = E2([t2^3, t2^3]); set_mordell_weil_basis!(Y2, [P2]);
julia> P2 = E2([tt^3, tt^3]); set_mordell_weil_basis!(Y2, [P2]);

julia> U2 = weierstrass_chart_on_minimal_model(Y2); U1 = weierstrass_chart_on_minimal_model(Y1);

julia> imgs = phi2.(phi1.(ambient_coordinates(U1))) # k(Y1) -> k(Y2)
3-element Vector{AbstractAlgebra.Generic.Frac{QQMPolyRingElem}}:
3-element Vector{AbstractAlgebra.Generic.FracFieldElem{QQMPolyRingElem}}:
(-(x//z)*t + t)//(x//z)^3
((x//z)*(y//z) - (y//z))//(x//z)^5
1//(x//z)
Expand All @@ -148,12 +165,12 @@ julia> set_attribute!(phi_rat, :is_isomorphism=>true);

julia> pullbackDivY1 = [pullback(phi_rat, D) for D in basisNSY1];

julia> B = [basis_representation(Y2, D) for D in pullbackDivY1];
julia> B = [basis_representation(Y2, D) for D in pullbackDivY1]
20-element Vector{Vector{QQFieldElem}}:
[4, 2, 0, 0, 0, 0, 0, 0, 0, -4, -4, -8, -7, -6, -5, -4, -3, -2, -1, 0]
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]
[4, 2, -1, -2, -3, -5//2, -2, -3//2, -3//2, -3, -5//2, -5, -9//2, -4, -7//2, -3, -5//2, -2, -3//2, -1]
[0, 0, 1, 2, 3, 5//2, 2, 3//2, 3//2, -2, -3//2, -3, -5//2, -2, -3//2, -1, -1//2, 0, 1//2, 1]
[4, 2, -1, -2, -3, -5//2, -2, -3//2, -3//2, -5//2, -3, -5, -9//2, -4, -7//2, -3, -5//2, -2, -3//2, -1]
[0, 0, 1, 2, 3, 5//2, 2, 3//2, 3//2, -3//2, -2, -3, -5//2, -2, -3//2, -1, -1//2, 0, 1//2, 1]
[0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[1, 0, 0, 0, 0, 0, 0, 0, 0, -1, -1, -2, -2, -2, -2, -2, -2, -2, -1, 0]
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0]
Expand All @@ -168,7 +185,7 @@ julia> B = [basis_representation(Y2, D) for D in pullbackDivY1];
[0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[2, 1, -1, -2, -3, -5//2, -2, -3//2, -3//2, -2, -5//2, -4, -7//2, -3, -5//2, -2, -3//2, -1, -1//2, 0]
[2, 1, -1, -2, -3, -5//2, -2, -3//2, -3//2, -5//2, -2, -4, -7//2, -3, -5//2, -2, -3//2, -1, -1//2, 0]
[0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

julia> B = matrix(QQ, 20, 20, reduce(vcat, B)); NSY2 = algebraic_lattice(Y2)[3];
Expand All @@ -181,8 +198,8 @@ julia> fibers_in_Y2 = [f[I]*B for f in fibers]
6-element Vector{Vector{QQFieldElem}}:
[4, 2, 0, 0, 0, 0, 0, 0, 0, -4, -4, -8, -7, -6, -5, -4, -3, -2, -1, 0]
[1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[5, 2, -2, -3, -4, -3, -2, -1, -2, -5, -4, -8, -7, -6, -5, -4, -3, -2, -1, 0]
[4, 2, -2, -4, -6, -9//2, -3, -3//2, -7//2, -3, -5//2, -5, -9//2, -4, -7//2, -3, -5//2, -2, -3//2, 0]
[5, 2, -2, -3, -4, -3, -2, -1, -2, -4, -5, -8, -7, -6, -5, -4, -3, -2, -1, 0]
[4, 2, -2, -4, -6, -9//2, -3, -3//2, -7//2, -5//2, -3, -5, -9//2, -4, -7//2, -3, -5//2, -2, -3//2, 0]
[2, 1, -1, -2, -3, -2, -1, 0, -2, -1, -1, -2, -2, -2, -2, -2, -2, -1, 0, 1]
[2, 1, 0, 0, 0, 0, 0, 0, 0, -2, -2, -4, -4, -4, -4, -3, -2, -1, 0, 1]

Expand All @@ -191,3 +208,50 @@ julia> f3 = fibers[3][I]; representative(elliptic_parameter(Y1, f3))

julia> g3a, phi3a = two_neighbor_step(Y1, f3); g3a
-x^3 + (-t^3 + 2)*x^2 - x + y^2

julia> @assert all(inner_product(ambient_space(NSY2), i,i) == 0 for i in fibers_in_Y2)

julia> [representative(elliptic_parameter(Y2, f)) for f in fibers_in_Y2[4:6]]
3-element Vector{AbstractAlgebra.Generic.FracFieldElem{QQMPolyRingElem}}:
(y//z)//((x//z)*t)
((y//z) + t^3)//((x//z)*t - t^4)
((y//z) + t^3)//((x//z) - t^3)

julia> g,_ = two_neighbor_step(Y2, fibers_in_Y2[4]);g
-1//4*x^4 - 1//2*t^2*x^3 - 1//4*t^4*x^2 + x + y^2

julia> R = parent(g); K_t = base_ring(R);

julia> (x,y) = gens(R); P = K_t.([0,0]); # rational point

julia> g, _ = transform_to_weierstrass(g, x, y, P);

julia> E4 = elliptic_curve(g, x, y)
Elliptic curve with equation
y^2 = x^3 + 1//4*t^4*x^2 - 1//2*t^2*x + 1//4

julia> g,_ = two_neighbor_step(Y2, fibers_in_Y2[5]);g
t^2*x^3 + (-1//4*t^4 + 2*t)*x^2 + x + y^2

julia> R = parent(g); K_t = base_ring(R);

julia> (x,y) = gens(R); P = K_t.([0,0]); # rational point

julia> g, _ = transform_to_weierstrass(g, x, y, P);

julia> E5 = elliptic_curve(g, x, y)
Elliptic curve with equation
y^2 = x^3 + (1//4*t^4 - 2*t)*x^2 + t^2*x

julia> g,_ = two_neighbor_step(Y2, fibers_in_Y2[6]);g
(t^2 + 2*t + 1)*x^3 + y^2 - 1//4*t^4

julia> R = parent(g); K_t = base_ring(R); t = gen(K_t);

julia> (x,y) = gens(R); P = K_t.([0,1//2*t^2]); # rational point

julia> g, _ = transform_to_weierstrass(g, x, y, P);

julia> E6 = elliptic_curve(g, x, y)
Elliptic curve with equation
y^2 + (-t^2 - 2*t - 1)//t^4*y = x^3
84 changes: 0 additions & 84 deletions test/book/specialized/brandhorst-zach-fibration-hopping/vinberg_3.jlcon
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions test/book/test.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ isdefined(Main, :FakeTerminals) || include(joinpath(pkgdir(REPL),"test","FakeTer
# these are skipped because they slow down the tests too much:

# sometimes very slow: 4000-30000s
"specialized/brandhorst-zach-fibration-hopping/vinberg_2.jlcon",
#"specialized/brandhorst-zach-fibration-hopping/vinberg_2.jlcon",
# very slow: 24000s
"cornerstones/number-theory/cohenlenstra.jlcon",
# ultra slow: time unknown
"specialized/markwig-ristau-schleis-faithful-tropicalization/eliminate_yz.jlcon",

# somewhat slow (~300s)
"cornerstones/polyhedral-geometry/ch-benchmark.jlcon",
"specialized/brandhorst-zach-fibration-hopping/vinberg_3.jlcon",
#"specialized/brandhorst-zach-fibration-hopping/vinberg_3.jlcon",
]

dispsize = (40, 130)
Expand Down
Loading