Two neighbor step in char 0 #4183

HechtiDerLachs · 2024-10-07T19:45:16Z

Building on #4177.

@simonbrandhorst : I managed to perform our two neighbor step in characteristic zero with this (modulo mistakes).

It would therefore be great if we could decide on how to integrate this code properly. I will leave some comments below.

experimental/Schemes/src/BlowupMorphism.jl

HechtiDerLachs · 2024-10-07T19:47:47Z

experimental/Schemes/src/elliptic_surface.jl

+  if !is_zero(P)
+    WC = weierstrass_chart_on_minimal_model(X)
+    U = original_chart(PX)
+    @assert OO(U) === ambient_coordinate_ring(WC)
+    PY = PrimeIdealSheafFromChart(X, WC, ideal(OO(WC), PX(U)))
+    set_attribute!(PY, :name, string("section: (",P[1]," : ",P[2]," : ",P[3],")"))
+    set_attribute!(PY, :_self_intersection, -euler_characteristic(X))
+    return WeilDivisor(PY, check=false)
+  end


I switched to the creation of sections from points to come from the weierstrass chart directly. This has the advantage that the strict transforms do not have to be computed to realize this divisor on that chart. This computation was killing us in char. 0, so I hope this is ok.

Previously, it was a PrimeIdealSheafFromChart. But I deliberately changed that to StrictTransformIdealSheaf because it was more performant (and in principle it allows the algorithms to take advantage of the blowup structure).
Unless you can provide evidence that your change improves performance in a wide range of examples please leave it as is. Optionally you could include another dispatch with a type as first argument.

One case where a PrimeIdealSheafFromChart will be faster is when we do only computations in the weierstrass chart e.g. for computing a linear system.

Exactly. The computations just didn't go through with the strict transform. Having a second argument as the return type should be OK, but we would still have to decide for a default.

HechtiDerLachs · 2024-10-07T19:49:22Z

experimental/Schemes/src/elliptic_surface.jl

+  mat2 = [constant_coefficient(numerator(x)) for x in mat]
+  M = matrix(B, length(eqns), length(ab), mat2)


This was making trouble over number fields:

ERROR: MethodError: no method matching (::AbsSimpleNumField)(::AbstractAlgebra.Generic.FracFieldElem{AbstractAlgebra.Generic.Poly{AbsSimpleNumFieldElem}}) Closest candidates are: (::AbsSimpleNumField)(::Singular.n_FieldElem{AbsSimpleNumFieldElem}) @ Singular ~/.julia/packages/Singular/lsYSW/src/MessyHacks.jl:119 (::AbsSimpleNumField)(::Vector{QQFieldElem}) @ Hecke ~/.julia/packages/Hecke/7wI0D/src/NumField/Elem.jl:284 (::AbsSimpleNumField)(::ZZRingElem) @ Nemo ~/.julia/packages/Nemo/tzyHK/src/antic/nf_elem.jl:1095 ...

I guess the implicit conversion via B was not very clean, anyway, so I tried to clear this up. @simonbrandhorst : Could you have a look whether this is now reasonable?

Your change looks like it should work.

Alternatively in line 1302 it could be
R,ab = polynomial_ring(Bt,vcat([Symbol(:a,i) for i in 0:dega],[Symbol(:b,i) for i in 0:degb]),cached=false)
instead (base was replaced by Bt)
because we don't get any denominators anyway.

experimental/Schemes/src/elliptic_surface.jl

HechtiDerLachs · 2024-10-07T19:55:09Z

experimental/Schemes/src/elliptic_surface.jl

+function reduction_to_pos_char(X::EllipticSurface, red_map::Map)
+  return get_attribute!(X, :reduction_to_pos_char) do
+    kk0 = base_ring(X)
+    @assert domain(red_map) === kk0
+    kkp = codomain(red_map)
+    @assert characteristic(kkp) > 0
+    _, result = base_change(red_map, X)
+    return red_map, result
+  end::Tuple{<:Map, <:Map}
+end


This is what we need to discuss. For elliptic surfaces it is sometimes reasonable to do computations of intersection numbers in a reduction modulo primes (as I hear). We probably do not want to allow this in general but have to hide it a bit from the user so that they use it at their own risk when they know what they're doing.

Instead of giving it as a second argument, I propose to have a field/attribute good_reduction which holds the reduction of the base_rings to positive characteristic, if it is set. Then the rationale is that a model of X in positive characteristic is held in the background, together with a list of counterparts for the generators of the algebraic_lattice. The methods to do intersection theory have to be customized in order to allow for several assumptions which can not be made unless we deal with elliptic surfaces. This is carried out in the code below. Someone with more background knowledge in elliptic surfaces (@simonbrandhorst ?) should check whether the code is safe to use, decide which precautions we require the user to take and how we want to communicate them.

experimental/Schemes/src/elliptic_surface.jl

HechtiDerLachs · 2024-10-07T19:57:17Z

experimental/Schemes/src/elliptic_surface.jl

+function _reduce_as_prime_divisor(bc::AbsCoveredSchemeMorphism, I::PrimeIdealSheafFromChart)
+  U = original_chart(I)
+  bc_cov = covering_morphism(bc)
+  V = __find_chart(U, codomain(bc_cov))
+  IV = I(V)
+  bc_loc = first(maps_with_given_codomain(bc_cov, V))
+  J = pullback(bc_loc)(IV)
+  set_attribute!(J, :is_prime=>true)
+  return PrimeIdealSheafFromChart(domain(bc), domain(bc_loc), J)
+end


This method assumes that the prime ideal sheaf I stays prime when reduced via bc. This is not automatically the case, so we should probably warn the user to only use "sufficiently good reduction" here.

experimental/Schemes/src/BlowupMorphism.jl

codecov · 2024-10-08T07:31:34Z

Codecov Report

Attention: Patch coverage is 38.84615% with 159 lines in your changes missing coverage. Please review.

Project coverage is 84.45%. Comparing base (020e83a) to head (52b58ba).
Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
experimental/Schemes/src/elliptic_surface.jl	35.89%	150 Missing ⚠️
.../AlgebraicGeometry/Schemes/Divisors/WeilDivisor.jl	25.00%	6 Missing ⚠️
experimental/Schemes/src/BlowupMorphism.jl	66.66%	2 Missing ⚠️
.../AlgebraicGeometry/Schemes/Sheaves/IdealSheaves.jl	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4183      +/-   ##
==========================================
- Coverage   84.59%   84.45%   -0.15%     
==========================================
  Files         631      640       +9     
  Lines       85085    85283     +198     
==========================================
+ Hits        71981    72029      +48     
- Misses      13104    13254     +150

Files with missing lines	Coverage Δ
...Morphisms/MorphismFromRationalFunctions/Methods.jl	`68.75% <100.00%> (-0.06%)`	⬇️
src/Rings/MPolyMap/MPolyRing.jl	`89.62% <100.00%> (-4.50%)`	⬇️
src/Rings/mpolyquo-localizations.jl	`73.48% <100.00%> (+0.02%)`	⬆️
.../AlgebraicGeometry/Schemes/Sheaves/IdealSheaves.jl	`80.41% <66.66%> (-0.19%)`	⬇️
experimental/Schemes/src/BlowupMorphism.jl	`84.15% <66.66%> (-0.60%)`	⬇️
.../AlgebraicGeometry/Schemes/Divisors/WeilDivisor.jl	`72.98% <25.00%> (+0.93%)`	⬆️
experimental/Schemes/src/elliptic_surface.jl	`79.97% <35.89%> (-6.08%)`	⬇️

... and 27 files with indirect coverage changes

src/AlgebraicGeometry/Schemes/Sheaves/IdealSheaves.jl

…hart

simonbrandhorst · 2024-10-30T14:00:10Z

@HechtiDerLachs it seems that some of your merges went wrong. The diff is completely off.

simonbrandhorst · 2024-10-31T12:43:31Z

src/AlgebraicGeometry/Schemes/Divisors/WeilDivisor.jl

@@ -338,7 +338,7 @@ function intersect(D::AbsWeilDivisor, E::AbsWeilDivisor;
        I = c1 + c2
        if !has_dimension_leq_zero(I) # potentially faster for localized ideals
          if c1 == c2
-            result = result + a1*a2*_self_intersection(c1)
+            result = result + a1*a2* (has_attribute(c1, :_self_intersection) ? _self_intersection(c1) : _self_intersection(c2))


why not put this in the function _self_intersection itself?

Because the function is called with one argument and does not know about the possibility that another object which is == to the given one might hold the information.

HechtiDerLachs commented Oct 7, 2024

View reviewed changes

HechtiDerLachs mentioned this pull request Oct 7, 2024

Elliptic surfaces fixes #4177

Merged

afkafkafk13 reviewed Oct 8, 2024

View reviewed changes

experimental/Schemes/src/BlowupMorphism.jl Outdated Show resolved Hide resolved

HechtiDerLachs added 5 commits October 8, 2024 09:53

Repair methods for has_dim_leq_zero.

1c46091

WIP to reduce to pos. char.

9684758

Make two_neighbor_step work in char. 0.

bc918bc

Remove debug comments.

af1e9c8

Take care of non-principal exceptional divisors.

a1c1f28

HechtiDerLachs force-pushed the two_neighbor_step_in_char_0 branch from 35374f6 to a1c1f28 Compare October 8, 2024 08:00

lgoettgens reviewed Oct 8, 2024

View reviewed changes

src/AlgebraicGeometry/Schemes/Sheaves/IdealSheaves.jl Show resolved Hide resolved

lgoettgens added the topic: schemes label Oct 8, 2024

HechtiDerLachs added 4 commits October 8, 2024 10:53

Clean up reduction to positive characteristic.

ff04310

Introduce a type of its own for sections and use it.

611adfc

Use AbsWeilDivisor consistently.

929ddac

More AbsWeilDivisor.

1e85bd3

HechtiDerLachs force-pushed the two_neighbor_step_in_char_0 branch from b4aa995 to 1e85bd3 Compare October 8, 2024 20:44

simonbrandhorst added 4 commits October 10, 2024 21:20

Merge branch 'master' into two_neighbor_step_in_char_0

bf57e5e

wip

c25dddb

Merge branch 'master' into two_neighbor_step_in_char_0

67a8d04

enable computation of admissible transformations on the weierstrass c…

f42e314

…hart

HechtiDerLachs force-pushed the two_neighbor_step_in_char_0 branch from 345b2f3 to f42e314 Compare October 30, 2024 15:02

HechtiDerLachs added 7 commits October 30, 2024 16:06

Extend _cmp_res and add assertion to mappings.

8ffb020

good_reduction -> raw_good_reduction.

0c348eb

Speed up mappings from quotient rings.

5e89ad1

Implement various reduction methods to pos. char..

d06d999

Turn primeness-check into an hassert.

7abad54

Add reduction method for automorphisms.

122cdf6

Add some assertions and vprints.

4e942fe

Allow more sources for self intersection.

3642189

simonbrandhorst reviewed Oct 31, 2024

View reviewed changes

HechtiDerLachs and others added 3 commits October 31, 2024 14:39

Add some more assertions.

ba3dece

Disable caching for cheap_realizations for the moment.

cc03158

one more AbsWeilDivisor

52b58ba

simonbrandhorst approved these changes Nov 1, 2024

View reviewed changes

simonbrandhorst enabled auto-merge (squash) November 1, 2024 12:26

simonbrandhorst merged commit 03b927f into oscar-system:master Nov 1, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two neighbor step in char 0 #4183

Two neighbor step in char 0 #4183

HechtiDerLachs commented Oct 7, 2024

HechtiDerLachs Oct 7, 2024

simonbrandhorst Oct 8, 2024

simonbrandhorst Oct 8, 2024

HechtiDerLachs Oct 8, 2024

HechtiDerLachs Oct 7, 2024

simonbrandhorst Oct 10, 2024

HechtiDerLachs Oct 7, 2024

HechtiDerLachs Oct 7, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

simonbrandhorst commented Oct 30, 2024

simonbrandhorst Oct 31, 2024

HechtiDerLachs Nov 1, 2024

		mat2 = [constant_coefficient(numerator(x)) for x in mat]
		M = matrix(B, length(eqns), length(ab), mat2)

Two neighbor step in char 0 #4183

Two neighbor step in char 0 #4183

Conversation

HechtiDerLachs commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

simonbrandhorst commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 8, 2024 •

edited

Loading