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.

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

Elliptic fibration book example errors #3881

Closed
joschmitt opened this issue Jun 21, 2024 · 16 comments
Closed

Elliptic fibration book example errors #3881

joschmitt opened this issue Jun 21, 2024 · 16 comments
Assignees
Labels
bug Something isn't working oscar book PRs necessary for the Oscar book

Comments

@joschmitt
Copy link
Member

The book example in the file specialized/brandhorst-zach-fibration-hopping/vinberg_3.jlcon errors both on OSCAR 1.1 and current master:

julia> K = QQ;

julia> Kt, t = polynomial_ring(K, :t);

julia> Ktf = fraction_field(Kt);

julia> E = elliptic_curve(Ktf, [0, -t^3, 0, t^3, 0]);

julia> P = E([t^3, t^3]);

julia> Y2 = elliptic_surface(E, 2, [P]);

julia> S = weierstrass_model(Y2)[1];

julia> basisNSY2, _, NSY2 = algebraic_lattice(Y2);

julia> fibers_in_Y2 = [QQ.(vec(collect(i))) for i in [
        [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],
        [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]
       ]];

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]]
ERROR: AssertionError: found
Stacktrace:
 [1] horizontal_decomposition(X::EllipticSurface{QQField, AbstractAlgebra.Generic.FracFieldElem{…}}, F::Vector{QQFieldElem})
   @ Oscar ~/.julia/dev/Oscar/experimental/Schemes/src/elliptic_surface.jl:1349
 [2] elliptic_parameter(X::EllipticSurface{QQField, AbstractAlgebra.Generic.FracFieldElem{…}}, F::Vector{QQFieldElem})
   @ Oscar ~/.julia/dev/Oscar/experimental/Schemes/src/elliptic_surface.jl:1419
 [3] (::var"#5#6")(f::Vector{QQFieldElem})
   @ Main ./none:0
 [4] iterate
   @ ./generator.jl:47 [inlined]
 [5] collect(itr::Base.Generator{Vector{Vector{QQFieldElem}}, var"#5#6"})
   @ Base ./array.jl:834
 [6] top-level scope
   @ REPL[12]:1
Some type information was truncated. Use `show(err)` to see complete types.

Notice that this is one of the examples that is not run by CI. (The code above runs for a few minutes.)

@joschmitt joschmitt added the bug Something isn't working label Jun 21, 2024
@fieker
Copy link
Contributor

fieker commented Jun 21, 2024 via email

@joschmitt
Copy link
Member Author

I tested it with the current master including the latest pull request from @simonbrandhorst and it didn't work. I'm happy to be proven wrong.

@joschmitt
Copy link
Member Author

For me, the error exists with the following versions:

julia> Oscar.versioninfo(full=true)
OSCAR version 1.2.0-DEV - #master, 5dcfffb955 -- 2024-06-21 12:51:22 +0200
  combining:
    AbstractAlgebra.jl   v0.41.9
    GAP.jl               v0.10.4
    Hecke.jl             v0.32.3
    Nemo.jl              v0.45.5
    Polymake.jl          v0.11.18
    Singular.jl          v0.23.2
  building on:
    FLINT_jll               v300.100.300+0
    GAP_jll                 v400.1200.200+9
    Singular_jll            v404.0.100+0
    libpolymake_julia_jll   v0.12.0+0
    libsingular_julia_jll   v0.45.1+0
    polymake_jll            v400.1200.1+0
See `]st -m` for a full list of dependencies.

Julia Version 1.10.4
Commit 48d4fd48430 (2024-06-04 10:41 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 20 × 13th Gen Intel(R) Core(TM) i5-13600
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, goldmont)
Threads: 1 default, 0 interactive, 1 GC (on 20 virtual cores)
Environment:
  JULIA_EDITOR = vim
Official https://julialang.org/ release

@thofma
Copy link
Collaborator

thofma commented Jun 21, 2024

Can confirm the findings of @joschmitt

@simonbrandhorst
Copy link
Collaborator

I can investigate. Though I am afraid the Book chapter wont keep running for long without CI.

@simonbrandhorst
Copy link
Collaborator

As far as I can tell, the order of the singular fibers changed.
It is just the order of factors in a factorization. As far as I know we do not guarantee the order of the factors to be fixed in any way.
At the time of writing of the chapter we overlooked this.
I think it cannot be fixed on the Oscar side, unless we want to fix the order of factors in a factorization. The book code is just not stable.
We could change the book code instead.

@HechtiDerLachs
Copy link
Collaborator

Btw: The code for our book-chapter was broken way before our recent changes in #3564 and #3877. Since it wasn't covered by CI we were unaware of various regressions caused by e.g. the lazyfication of ideal sheaves. If at the state now it's really only about the order of factorization, I think we're as good as we can possibly be again.

@simonbrandhorst
Copy link
Collaborator

Since there is no easy fix, let's just ignore it.

@joschmitt

This comment was marked as outdated.

@fingolfin
Copy link
Member

I don't think we can completely ignore it. Let me be clear: I fully agree that this is not something to worry about before e.g. the Begehung.

But for the OSCAR book we should have a working example. At the very least, the book website would have to point out if the example is broken.

Ideal would be if the book example could get a small adjustment to make it work in OSCAR 1.0, 1.1 and master / 1.x -- if that's possible, let's do it, we still have some time to make minor edits to the book.

If this is impossible or too hard, we need to make a decision: must the OSCAR book examples work in 1.0 (then leave it, and use the website to communicate that a change is needed for the code in 1.1 and above); or is it OK to have the example in that chapter require 1.1 (perhaps with a footnote stating that). But in any case, both are inferior to the solution with an adjust example that works everywhere.

Anyway, I am reopening this to remind us to deal with the book before the final version is submitted to Springer.

@fingolfin fingolfin reopened this Jun 26, 2024
@lgoettgens lgoettgens added the oscar book PRs necessary for the Oscar book label Jun 26, 2024
@simonbrandhorst
Copy link
Collaborator

This is not doable in 1.0. In 1.1.1 it should be possible. Please let us know asap.

@joschmitt
Copy link
Member Author

This is not doable in 1.0. In 1.1.1 it should be possible. Please let us know asap.

Let who know what?

@HechtiDerLachs
Copy link
Collaborator

The proofs for the Oscar book are coming up, probably during the next week. If we shall adjust the code for 1.1.1 or the snippets in the book chapter, please let us know which final behavior you are expecting.

@simonbrandhorst
Copy link
Collaborator

#3939 is an attempt at resolving this.

@lgoettgens
Copy link
Member

Is there anything else to do with this issue after #3939 or can this get closed?

@simonbrandhorst
Copy link
Collaborator

From my side this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working oscar book PRs necessary for the Oscar book
Projects
None yet
Development

No branches or pull requests

7 participants