Fix FTheoryTools bug, extend strict_transform

[paemurru]

Fixes a bug in FTheoryTools introduced by #4484. Namely, it previously computed the total transform, not the strict transform.

Even with the speed improvements of #4485, computing the strict transform with ideals is slower than operating on polynomials. Therefore, I added strict and total transform functions on polynomials. Also added corresponding tests.

This has only a small impact on the benchmarks in #4484:

julia> @btime h_5 = resolve(h, 1)
  177.930 s (38081849 allocations: 7.81 GiB)
Partially resolved global Tate model over a concrete base -- The F-theory geometry with most flux vacua based on arXiv paper 1511.03209 Eq. (2.11)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.50%. Comparing base (d5e963e) to head (df354a2).
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4503      +/-   ##
==========================================
+ Coverage   84.16%   84.50%   +0.33%     
==========================================
  Files         672      673       +1     
  Lines       88768    90171    +1403     
==========================================
+ Hits        74715    76196    +1481     
+ Misses      14053    13975      -78     

Files with missing lines	Coverage Δ
.../FTheoryTools/src/AbstractFTheoryModels/methods.jl	77.52% <100.00%> (+0.13%)	⬆️
experimental/Schemes/src/ToricBlowups/methods.jl	97.01% <100.00%> (+0.93%)	⬆️

... and 53 files with indirect coverage changes

[HereAround]

Thank you for spotting and working on this @paemurru . Generally speaking, your changes look good to me.

Here are a few points that caught my attention:

• Unless I am mistaken, the function total_transform(f::ToricBlowupMorphism, I::Union{MPolyIdeal, MPolyDecRingElem}) is not type stable (a topic I understood you cared deeply about in the past?), as its return value could be either a polynomial or an ideal. Maybe we could have two variants, one for a polynomial as input and return value, and the second variant for an ideal as input/output? This would mimic function strict_transform(f::ToricBlowupMorphism, I::MPolyIdeal) and function strict_transform(f::ToricBlowupMorphism, g::MPolyDecRingElem), as well as their counterparts for strict_transform_with_index.
• If you agree to the above, consider adding a docstring to both methods. You could simply use the examples that you currently propose in runtests.jl, but with the benefit that users would see that indeed both inputs/outputs (ideal, polynomial) are possible. (This is clearly minor.)
• I am confused by your smooth/simplicial requirements. For strict_transform you allow arbitrary singularities, for total_transform at most orbifold-singularities are ok, and for strict_transform_with_index you require the absence of any singularities. I am not sure if I understand this.
• Could we have a test/requirement which errors in case the ideal/polynomial provided do not belong to the Cox ring of the codomain of the blowdown morphism? If not, at what point will the method fail?

[HereAround]

Stupid question... but still: What does this line do?

[paemurru]

This builds the docstring for both when the argument is an MPolyIdeal and a MPolyDecRingElem. Without having to copy the docstring for both of the functions.

[fingolfin]

I don't understand this rationale. The docstring already now shows up in the REPL when one does ?strict_transform_with_index regardless of how many methods you add.

Likewise it shows up in the manual already now, and it will keep appearing even though you added another method.

On the other hand, I am not sure it will still appear in the manual with this change, did you change that? Right now docs/src/AlgebraicGeometry/ToricVarieties/BlowupMorphisms.md includes the docstring via

strict_transform_with_index(f::ToricBlowupMorphism, I::MPolyIdeal)

and given the arcane nature of how Documenter.jl matches these things I am not sure your modified versions still matches.

All in all it would be much simpler to not add this line?

[paemurru]

Removed the line with Union{...}. I hope this is what you meant.

[paemurru]

@HereAround Thank you for your comments! I will try to answer them as best I can.

• The function total_transform(f::ToricBlowupMorphism, I::Union{MPolyIdeal, MPolyDecRingElem}) is actually type stable. This line is just to build the docstring.
• (see above)
• Yes, these are all correct. (I can explain longer in a call or in Slack)
• It actually already tests this:

julia> X = affine_space(NormalToricVariety, 2)
Normal toric variety

julia> phi = blow_up(X, [2, 3])
Toric blowup morphism

julia> R = first(graded_polynomial_ring(QQ, ["x", "y"]))
Multivariate polynomial ring in 2 variables over QQ graded by
  x -> [1]
  y -> [1]

julia> x, y = gens(R)
2-element Vector{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}:
 x
 y

julia> strict_transform(phi, ideal(R, [x*y]))
ERROR: ArgumentError: g must be an element of the Cox ring of the codomain of f

[HereAround]

@HereAround Thank you for your comments! I will try to answer them as best I can.

* The function `total_transform(f::ToricBlowupMorphism, I::Union{MPolyIdeal, MPolyDecRingElem})` is actually type stable. This line is just to build the docstring.

* (see above)

* Yes, these are all correct. (I can explain longer in a call or in Slack)

* It actually already tests this:

julia> X = affine_space(NormalToricVariety, 2)
Normal toric variety

julia> phi = blow_up(X, [2, 3])
Toric blowup morphism

julia> R = first(graded_polynomial_ring(QQ, ["x", "y"]))
Multivariate polynomial ring in 2 variables over QQ graded by
  x -> [1]
  y -> [1]

julia> x, y = gens(R)
2-element Vector{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}:
 x
 y

julia> strict_transform(phi, ideal(R, [x*y]))
ERROR: ArgumentError: g must be an element of the Cox ring of the codomain of f

Thank you for explaining @paemurru ! I will look carefully at your PR by the light of day tomorrow.

Re your example: So the "belongs to the Cox ring"-test is part of another called function (maybe cox_ring_homomorphism). That is good!

[HereAround]

Looks good to me @paemurru . Thank you for working on this!

One (hopefully) minor issue: I see that a required test failed after 150 minutes (https://github.com/oscar-system/Oscar.jl/actions/runs/13036857121/job/36369371736?pr=4503). To me this appears to be "just" a time-out error (coincidentally terminated after an FTheoryTools test). Once the test runs complete, I suggest to restart the failed jobs.

[paemurru]

Yes, please restart the required failed test, I do not have sufficient access rights to do this myself