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

Elliptic surfaces fixes #4177

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Elliptic surfaces fixes #4177

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2dd38da
Disable some internal checks in elliptic_surface.
HechtiDerLachs Oct 4, 2024
f4df3ad
Repair methods for has_dim_leq_zero.
HechtiDerLachs Oct 4, 2024
493a6c9
Fix up decomposition_info.
HechtiDerLachs Oct 4, 2024
6b5ffbd
Fix a test.
HechtiDerLachs Oct 4, 2024
4156b51
Remove old code.
HechtiDerLachs Oct 7, 2024
2f03447
Apply suggestions from review.
HechtiDerLachs Oct 7, 2024
fc4d29b
Switch to hopefully more economic saturation.
HechtiDerLachs Oct 7, 2024
d6d4236
Add type stability.
HechtiDerLachs Oct 7, 2024
03df499
Add possibility to compute intersection numbers in pos. char.
HechtiDerLachs Oct 7, 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
2 changes: 1 addition & 1 deletion experimental/Schemes/src/elliptic_surface.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1213,7 +1213,7 @@ function _section_on_weierstrass_ambient_space(X::EllipticSurface, P::EllipticCu
U = X0[1][1]
(x,y,t) = coordinates(U)
b = P
return ideal_sheaf(X0,U,[OO(U)(i) for i in [x*denominator(b[1])(t)-numerator(b[1])(t),y*denominator(b[2])(t)-numerator(b[2])(t)]])
return ideal_sheaf(X0,U,[OO(U)(i) for i in [x*denominator(b[1])(t)-numerator(b[1])(t),y*denominator(b[2])(t)-numerator(b[2])(t)]]; check=false)
end

function _section(X::EllipticSurface, P::EllipticCurvePoint)
Expand Down
16 changes: 10 additions & 6 deletions src/AlgebraicGeometry/Schemes/Covering/Objects/Attributes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,18 +107,22 @@

for V in patches(orig_cov)
# Collect the patches in `ref_cov` refining V
V_ref = [U for U in patches(ref_cov) if decomp_dict[U][1] === V]
# Collect the corresponding complement equations
comp_eqns = [decomp_dict[U][2] for U in V_ref]
V_ref = [(U, (UV, h)) for (U, (UV, h)) in decomp_dict if UV === V]
_compl(a) = sum(length(lifted_numerator(b)) + length(lifted_denominator(b)) for b in a[2][2]; init=0)
Copy link
Member

Choose a reason for hiding this comment

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

A minor and purely optional nitpick (feel free to ignore):

So UV is always V. So why not do this?

Suggested change
V_ref = [(U, (UV, h)) for (U, (UV, h)) in decomp_dict if UV === V]
_compl(a) = sum(length(lifted_numerator(b)) + length(lifted_denominator(b)) for b in a[2][2]; init=0)
V_ref = [(U, h) for (U, (UV, h)) in decomp_dict if UV === V]
_compl(a) = sum(length(lifted_numerator(b)) + length(lifted_denominator(b)) for b in a[2]; init=0)

and then the for loop in line 115 can be slightly simplified, and also line 120.

This avoids a little bit of storage space for those tuples.

Anyway, perhaps you have a reason for having it as it is (e.g. plans for further changes, keeping the code similar to something else etc.) -- so as I said, feel free to ignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes sense. I just cluttered it together so that it would work for the moment. Thanks for the close look.

V_ref = sort!(V_ref, by=_compl)
# Start out from the original decomposition info
dec_inf = copy(decomposition_info(orig_cov)[V])
for (i, U) in enumerate(V_ref)
for (i, (U, (_, h))) in enumerate(V_ref)
HechtiDerLachs marked this conversation as resolved.
Show resolved Hide resolved
# Cast the already made decomposition info down to U
tmp = elem_type(OO(U))[OX(V, U)(i) for i in dec_inf] # help the compiler
# Append the equations for the previously covered patches
for j in 1:i-1
W = V_ref[j]
surplus = OX(V, U).(decomp_dict[W][2])
(_, (_, hh)) = V_ref[j]
HechtiDerLachs marked this conversation as resolved.
Show resolved Hide resolved
if is_empty(hh) # The patch for hh already covered everything; this one can hence be discarded.
push!(tmp, one(OO(U)))
break

Check warning on line 123 in src/AlgebraicGeometry/Schemes/Covering/Objects/Attributes.jl

View check run for this annotation

Codecov / codecov/patch

src/AlgebraicGeometry/Schemes/Covering/Objects/Attributes.jl#L122-L123 

Added lines #L122 - L123 were not covered by tests
end
surplus = OX(V, U).(hh)
push!(tmp, prod(surplus; init=one(OO(U))))
end
set_decomposition_info!(ref_cov, U, tmp)
Expand Down
13 changes: 0 additions & 13 deletions src/AlgebraicGeometry/Schemes/Sheaves/CoherentSheaves.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,19 +784,6 @@ end
return C
end

function inherit_decomposition_info!(C::Covering, X::AbsCoveredScheme)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why remove this? Was it a duplicate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. And it was wrong. Fortunately, it seemed to be used nowhere.

D = default_covering(X)
OOX = OO(X)
if has_decomposition_info(D)
for U in patches(C)
V = __find_chart(U, D)
phi = OOX(V, U)
set_decomposition_info!(C, U, phi.(decomposition_info(D)[V]))
end
end
return C
end

@attr Covering function trivializing_covering(M::HomSheaf)
X = scheme(M)
OOX = OO(X)
Expand Down
6 changes: 3 additions & 3 deletions src/AlgebraicGeometry/Schemes/Sheaves/IdealSheaves.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ end
end

@attr Bool function has_dimension_leq_zero(I::Ideal)
is_one(I) && return true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's superfluous. It's usually not easier to compute this than the dimension. Both are the same Groebner basis computation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep. Is the GB stored? Or could it be that it is discarded and just the is_one attribute is true?

#is_one(I) && return true
HechtiDerLachs marked this conversation as resolved.
Show resolved Hide resolved
return dim(I) <= 0
end

Expand All @@ -354,14 +354,14 @@ end
P = base_ring(R)::MPolyRing
J = ideal(P, numerator.(gens(I)))
has_dimension_leq_zero(J) && return true
is_one(I) && return true
#is_one(I) && return true
return dim(I) <= 0
end

@attr Bool function has_dimension_leq_zero(I::MPolyQuoLocalizedIdeal)
R = base_ring(I)
P = base_ring(R)::MPolyRing
J = ideal(P, lifted_numerator.(gens(I)))
J = pre_saturated_ideal(pre_image_ideal(I))
has_dimension_leq_zero(J) && return true
is_one(I) && return true
return dim(I) <= 0
Expand Down
2 changes: 1 addition & 1 deletion test/AlgebraicGeometry/Schemes/CoveredScheme.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@
new_cov = Covering(append!(AbsAffineScheme[V1, V2], patches(orig_cov)[2:end]))
Oscar.inherit_gluings!(new_cov, orig_cov)
Oscar.inherit_decomposition_info!(X, new_cov, orig_cov=orig_cov)
@test Oscar.decomposition_info(new_cov)[V2] == [OO(V2)(x-1)]
@test Oscar.decomposition_info(new_cov)[V1] == [OO(V1)(x)]
end

@testset "fiber products of coverings" begin
Expand Down
Loading