Check for unnecessary uses of underlying_scheme

[benlorenz]

Describe the bug
The show method for EllipticSurface can be extremely slow:

Oscar.jl/experimental/Schemes/elliptic_surface.jl

Lines 238 to 269 in b7b63dc

	@doc raw"""
	#Examples
	```jldoctest
	julia> Qt, t = polynomial_ring(QQ, :t);
	julia> Qtf = fraction_field(Qt);
	julia> E = EllipticCurve(Qtf, [0,0,0,0,t^5*(t-1)^2]);
	julia> X3 = elliptic_surface(E, 2)
	Elliptic surface
	over rational field
	with generic fiber
	-x^3 + y^2 - t^7 + 2*t^6 - t^5
	and relatively minimal model
	scheme over QQ covered with 45 patches
	```
	"""
	function Base.show(io::IO, ::MIME"text/plain", S::EllipticSurface)
	io = pretty(io)
	println(io, "Elliptic surface")
	println(io, Indent(), "over ", Lowercase(), base_ring(S))
	println(io, Dedent(), "with generic fiber")
	print(io, Indent(), Lowercase(), equation(generic_fiber(S)), Dedent())
	if isdefined(S, :Y)
	println(io)
	println(io, "and relatively minimal model")
	print(io, Indent(), Lowercase(), relatively_minimal_model(S)[1], Dedent())
	end
	print(io, Dedent())
	end

To Reproduce
Run the code from the doctest in elliptic_surface.jl:241-253:

julia> Qt, t = polynomial_ring(QQ, :t);

julia> Qtf = fraction_field(Qt);

julia> E = EllipticCurve(Qtf, [0,0,0,0,t^5*(t-1)^2]);

julia> X3 = elliptic_surface(E, 2)
Elliptic surface
  over rational field
with generic fiber
  -x^3 + y^2 - t^7 + 2*t^6 - t^5
and relatively minimal model
  scheme over QQ covered with 45 patches

The last step takes about 200 seconds including compilation time. After recreating the object it takes "only" about 60 seconds (i.e. excluding compilation time)

More precisely it happens due to show calling base_ring:

julia> Qt, t = polynomial_ring(QQ, :t);

julia> Qtf = fraction_field(Qt);

julia> E = EllipticCurve(Qtf, [0,0,0,0,t^5*(t-1)^2]);

julia> X3 = elliptic_surface(E, 2);

julia> @time base_ring(X3)
213.849025 seconds (486.34 M allocations: 23.844 GiB, 6.70% gc time, 62.53% compilation time)
Rational field

julia> X3 = elliptic_surface(E, 2);

julia> @time base_ring(X3)
 77.345964 seconds (331.51 M allocations: 13.487 GiB, 14.47% gc time, 0.03% compilation time)
Rational field

Expected behavior
If this method can take that long it should not be used in the show method and maybe not in the doctests as well. Maybe there is an easier / faster way to determine the base_ring ?

Also I don't think it is a good idea to attach an example to the docstring of Base.show, I can't find it in our documentation.

This was added in #2580, cc: @StevellM

[afkafkafk13]

@StevellM Thank you for adressing the problem immediately.
Your PR adresses the immediate problem of the slow doctests, but we need to keep this issue open, because base_ring for a scheme over QQ should not be so extremely slow. This is not on your plate, but should be considered in a wider scope, when we look at time and space consumption of the scheme framework.(@simonbrandhorst @HechtiDerLachs )

[simonbrandhorst]

@benlorenz I asked Stevell to put in the docstring of the show methods so that it is tested by our CI. Which apparently helped to find this bug. Do you see a better way to test that all the printing works as intended?

[HechtiDerLachs]

I think the problem is that by default a call to `base_ring` first realizes the full `underlying_scheme`. Indeed, an easier workaround should be to add `base_ring(X::EllipticSurface) = coefficient_ring(base_field(elliptic_curve(X)))` or something similar, i.e. to infer the base ring from the elliptic curve for `X`.

…

On July 25, 2023 8:35:47 AM GMT+01:00, simonbrandhorst ***@***.***> wrote: @benlorenz I asked Stevell to put in the docstring of the show methods so that it is tested by our CI. Which apparently helped to find this bug. Do you see a better way to test that all the printing works as intended? -- Reply to this email directly or view it on GitHub: #2584 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[fieker]

Anne wanted to reopen this...

[fieker]

Anne @afkafkafk13 is analysing and managing this further...

[simonbrandhorst]

@afkafkafk13 In this special case the reason is that underlying_scheme constructs a (covered) scheme.
Depending on what the underlying_scheme is, this can be more or less expensive.
The obivous way to treat it, is to implement base_ring for the subtype of scheme that one considers.
But I am not aware of any more generic or easier solution.

[afkafkafk13]

For this occurrence of the problem, I fully agree with you.
I wanted this issue reopened as a marker for the following closely related tasks

• check further uses of underlying_scheme to identify instances where this unnecessarily creates the scheme and can be avoided
• document this pitfall, because of the high probability of getting into similar settings in most applications built on top of the schemes framework

[fingolfin]

Regarding how to test the printing, there are multiple options:

1. add doctests exercising the printing to the docstrings of other methods (if it makes sense, i.e., fits with the method being documented)
2. add doctests to the manual chapter for elliptic surfaces (if it makes sense)
3. add a regular test under test/ which exercises the printing -- it can also use a doctest, see test/Groups/group_characters.jl for an example)