[ToricVarieties] Enhance blow_up method #2741

HechtiDerLachs · 2023-08-29T10:22:48Z

@HereAround : Can you take it from here?

src/AlgebraicGeometry/ToricVarieties/ToricSchemes/attributes.jl

HereAround · 2023-08-29T14:44:17Z

@HereAround : Can you take it from here?

Working on it.

HereAround · 2023-08-30T13:17:06Z

@lkastner This is the next "round" of toric schemes and their applications in the FTheoryTools.

In this PR, @HechtiDerLachs and I extend the existing blow_up functionality for toric varieties. In the current master, these blowups only work provided that the blowup can be conducted (to the best of my knowledge) torically. This PR addresses the complement. Namely, if the blowup cannot (to my knowledge) be conducted torically, then we call the corresponding schemes functionality to perform the blowup with the techniques that @HechtiDerLachs implemented. This is what @apturner is very much looking forward to making use of for the FTheoryTools.

Once the tests complete, this is - as I see it - ready for review.

HechtiDerLachs · 2023-08-30T14:13:48Z

So far, it doesn't seem to be our fault, that the tests fail. One is due to singular conversion of finite fields and the other is due to some doctest in the GIT fans. @jankoboehm : Do you maybe see what's going on?

HereAround · 2023-08-30T16:43:24Z

So far, it doesn't seem to be our fault, that the tests fail. One is due to singular conversion of finite fields and the other is due to some doctest in the GIT fans. @jankoboehm : Do you maybe see what's going on?

At least one failure happened because of me - I wrote test instead of testset by accident. Let see what the tests say now.

HereAround · 2023-08-30T19:15:29Z

It seems that now all required tests pass but the doctest (cf. https://github.com/oscar-system/Oscar.jl/actions/runs/6027670947/job/16353354055?pr=2741). I am not sure (yet) why the latter failed...

benlorenz · 2023-08-30T20:26:31Z

It seems that now all required tests pass but the doctest (cf. oscar-system/Oscar.jl/actions/runs/6027670947/job/16353354055?pr=2741). I am not sure (yet) why the latter failed...

There is a whitespace difference in the output here: https://github.com/oscar-system/Oscar.jl/actions/runs/6027670947/job/16353354443?pr=2741#step:8:2020

┌ Error: doctest failure in ~/work/Oscar.jl/Oscar.jl/src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/standard_constructions.jl:300-353
...
│ Subexpression:
│ 
│ I2 = ideal([x2 * x3])
│ 
│ Evaluated output:
│ 
│ ideal(x2*x3)
│ 
│ Expected output:
│ 
│ ideal(x2 * x3)
│ 
│   diff = ideal(x2 * x3)ideal(x2*x3)
└ @ Documenter.DocTests

Unfortunately that error block is now somewhat hard to find due to the extra output. I did not expect the doctests to continue running when encountering an error...
I will have a look if we can improve this.

HereAround · 2023-08-30T20:33:51Z

It seems that now all required tests pass but the doctest (cf. oscar-system/Oscar.jl/actions/runs/6027670947/job/16353354055?pr=2741). I am not sure (yet) why the latter failed...

There is a whitespace difference in the output here: https://github.com/oscar-system/Oscar.jl/actions/runs/6027670947/job/16353354443?pr=2741#step:8:2020
┌ Error: doctest failure in ~/work/Oscar.jl/Oscar.jl/src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/standard_constructions.jl:300-353
...
│ Subexpression:
│ 
│ I2 = ideal([x2 * x3])
│ 
│ Evaluated output:
│ 
│ ideal(x2*x3)
│ 
│ Expected output:
│ 
│ ideal(x2 * x3)
│ 
│   diff = ideal(x2 * x3)ideal(x2*x3)
└ @ Documenter.DocTests 
Unfortunately that error block is now somewhat hard to find due to the extra output. I did not expect the doctests to continue running when encountering an error... I will have a look if we can improve this.

Thank you. Fixed and just updated the code.

For my curiosity: How did you find this? I looked at the log from top to bottom but did not spot this. Did I just overlook it?

benlorenz · 2023-08-30T20:40:58Z

I searched for failure in the box on the top right (with the corresponding log block expanded).

codecov · 2023-08-30T22:59:42Z

Codecov Report

Merging #2741 (75bfdaf) into master (dfab548) will increase coverage by 0.09%.
The diff coverage is 90.67%.

@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
+ Coverage   73.33%   73.43%   +0.09%     
==========================================
  Files         451      451              
  Lines       64167    64240      +73     
==========================================
+ Hits        47058    47175     +117     
+ Misses      17109    17065      -44

Files Changed	Coverage
...ies/NormalToricVarieties/standard_constructions.jl	`0.00%`
src/deprecations.jl	`ø`
src/PolyhedralGeometry/iterators.jl	`66.66%`
experimental/Schemes/IdealSheaves.jl	`92.85%`
.../ToricVarieties/NormalToricVarieties/attributes.jl	`100.00%`
...Geometry/ToricVarieties/ToricSchemes/attributes.jl	`100.00%`

experimental/Schemes/IdealSheaves.jl

HereAround · 2023-08-31T08:52:07Z

@HechtiDerLachs, based on our conversations in slack, I now take the liberty to mark this as ready for review. Please correct me if needed.

lkastner

This is moving a bit too fast for me to keep up with. There is already a lot to unpack below the mathematical level. Try to add more tests for boundary cases.

lkastner · 2023-08-31T08:21:22Z

experimental/Schemes/IdealSheaves.jl

+  for (k, U) in enumerate(affine_charts(X))
+
+    # We first create the morphism Φₛ* from p. 224, l. 3.
+    indices = [j for j in 1:nrays(X) if ray_indices(maximal_cones(X))[k,j]]


This is again an abuse of IncidenceMatrix.

julia> nf = normal_fan(cube(2)) Polyhedral fan in ambient dimension 2 julia> IM = maximal_cones(IncidenceMatrix, nf) 4×4 IncidenceMatrix [1, 3] [2, 3] [1, 4] [2, 4] julia> row(IM, 1) Set{Int64} with 2 elements: 3 1 julia> 1 in row(IM, 1) true julia> 2 in row(IM, 1) false

Sorry for this abuse. I of course read your example and adjusted the code a bit. But I am not sure if I did grasp the full scale of how this can be optimized with your proposal. Please have a look at 730c7e0 and let me know what you think.

lkastner · 2023-08-31T08:39:29Z

experimental/Schemes/IdealSheaves.jl

+
+    # Now we need to create the inverse of α*.
+    imgs_alpha_star = elem_type(help_ring)[]
+    for m in hilbert_basis(dual_cone(U))


Why is there something called dual_cone? It was agreed upon that the function taking cones to their (polar) dual is called polarize. But this function cannot even be applied to cones, only to affine normal toric varieties. So it is even worse, it will confuse users who only want to dualize a polyhedral cone and users having an affine normal toric variety will not know that this might be the function they are looking for.

There are better names that one can choose, please have a look in CLS. In polymake we called this the WEIGHT_CONE, but also something like semigroup_cone or character_cone could probably work. dual_cone is definitely not ok.

My recollection is a bit vague - I believe this dates back probably half a year or even longer, when we first prototyped the toric schemes. The ideas did not really go very deep, but focused on organizing the code with some helper functions. Nothing more than that. This is I think why this exists in the first place.

Certainly, I am more than happy to rename it. Based on what you are saying, maybe it would be best to align it with the polymake name, i.e. weight_cone. Let me prepare that...

Proposal is here 0f159c6. What do you think?

lkastner · 2023-08-31T08:40:57Z

experimental/Schemes/IdealSheaves.jl

+
+    # Now we need to create the inverse of α*.
+    imgs_alpha_star = elem_type(help_ring)[]
+    for m in hilbert_basis(dual_cone(U))


What if this cone has lineality?

julia> c = positive_hull([1 0; -1 0]) Polyhedral cone in ambient dimension 2 julia> hilbert_basis(c) ERROR: ArgumentError: Cone not pointed Stacktrace: [1] macro expansion @ ~/.julia/packages/Hecke/xt25Z/src/Assertions.jl:564 [inlined] [2] hilbert_basis(C::Cone{QQFieldElem}) @ Oscar ~/oscars/master/src/PolyhedralGeometry/Cone/properties.jl:529 [3] top-level scope @ REPL[14]:1

I think you probably also want to require at least isfulldimensional and maybe also ispointed.

Thank you for the pointers. Proposal for resolution is here: 0cc754c

lkastner · 2023-08-31T09:11:49Z

experimental/Schemes/IdealSheaves.jl

+function _generic_blow_up(v::Any, I::Any)
+  error("Not yet supported")
+end


Suggested change

function _generic_blow_up(v::Any, I::Any)

error("Not yet supported")

end

Why is this needed?

The code on ideal sheaves is still experimental, while the blow_up function in the toric varieties is in src.

I understand that there was a rule "src must never call experimental" or similar. I discussed this with @fingolfin yesterday, and he proposed this work-around. It ensure that even if you disabled all experimental code, the toric varieties code would still run except for raising the error "Not yet supported" exactly where the call into experimental would have happened.

I don't understand. If this wasn't here you would also get an error, right? Also this function is in experimental, so it would be disabled too if experimental is gone?

I also noticed this and adjusted it. Let me push the changes that I have by now. One sec.

If I did it correctly, then this function should now be in standard_constructions.jl, right below the blow_up function.

And yes, if this function was not there, we would also get an error ("something undefined" or alike). Maybe the "Not yet supported" error is better? Maybe not? Maybe the error message could be adjusted...

As per usual - I am happy to hear other/better alternatives. :)

lkastner · 2023-08-31T09:23:00Z

experimental/Schemes/IdealSheaves.jl

+  # We need to dehomogenize the ideal I ⊂ S = ℂ[xᵨ| ρ ∈ Σ(1)] to the local 
+  # charts Uₛ, s ⊂ Σ a cone. To this end we use Proposition 5.2.10 from 
+  # Cox-Little-Schenck, p. 223ff. Abstractly, the chart 
+  # Uₛ is isomorphic to ℂˢ⁽¹⁾ = Spec(ℂ[xᵨ | ρ ∈ s(1)]). But these are 
+  # not the coordinates we use for the affine charts here. They 
+  # correspond to the `hilbert_basis(dual_cone(U_s))`.


https://docs.oscar-system.org/dev/DeveloperDocumentation/styleguide/

the use of Unicode should be kept to a minimum.

Let me modify this... (cc @HechtiDerLachs).

I am rebasing to work in those changes and will push shortly. Please inspect again and let me know if this is better.

My understanding is that @HechtiDerLachs uses Unicode for the documentation and comments in the schemes code. Certainly, I have only touched the lines about the ideal sheaves added in this PR.

https://docs.oscar-system.org/dev/DeveloperDocumentation/styleguide/

the use of Unicode should be kept to a minimum.

I'm using the notation from the book and tried to stick as close to it as possible so people do not have to go through the hustle to write down a dictionary to translate the comment to the setup of CLS. I think this is a justified use of unicode.

experimental/Schemes/IdealSheaves.jl

lkastner · 2023-08-31T09:29:53Z

experimental/Schemes/IdealSheaves.jl

+julia> IdealSheaf(P3, I)
+Sheaf of ideals
+  on scheme over QQ covered with 4 patches
+    1: [x_1_1, x_2_1, x_3_1]   normal, affine toric variety
+    2: [x_1_2, x_2_2, x_3_2]   normal, affine toric variety
+    3: [x_1_3, x_2_3, x_3_3]   normal, affine toric variety
+    4: [x_1_4, x_2_4, x_3_4]   normal, affine toric variety
+with restrictions
+  1: ideal(x_2_1, x_3_1)
+  2: ideal(x_2_2, x_3_2)
+  3: ideal(1, x_3_3)
+  4: ideal(x_2_4, 1)


I don't think this output is sustainable for larger examples.

Ok, let me simply suppressed the output for now:

julia> IdealSheaf(P3, I);

I don't think this output is sustainable for larger examples.

This has been thought through by @simonbrandhorst and thoroughly worked out by @StevellM . If you have doubts about this, we can contact them, but let's not open this discussion on this pr.

If an object is complicated, it may take much space to print. If the output is too large, one can still use a semicolon to suppress it or ask for compact printing.

lkastner · 2023-08-31T09:31:35Z

src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/standard_constructions.jl

+julia> b2P3 = blow_up(P3, I2)
+Blow up
+  of scheme over QQ covered with 4 patches
+    1b: [x_1_1, x_2_1, x_3_1]   normal, affine toric variety
+    2b: [x_1_2, x_2_2, x_3_2]   normal, affine toric variety
+    3b: [x_1_3, x_2_3, x_3_3]   normal, affine toric variety
+    4b: [x_1_4, x_2_4, x_3_4]   normal, affine toric variety
+  in sheaf of ideals with restrictions
+    1b: ideal(x_2_1*x_3_1)
+    2b: ideal(x_2_2*x_3_2)
+    3b: ideal(x_3_3)
+    4b: ideal(x_2_4)
+with domain
+  scheme over QQ covered with 4 patches
+    1a: [x_1_1, x_2_1, x_3_1]   spec of quotient of multivariate polynomial ring
+    2a: [x_1_2, x_2_2, x_3_2]   spec of quotient of multivariate polynomial ring
+    3a: [x_1_3, x_2_3, x_3_3]   spec of quotient of multivariate polynomial ring
+    4a: [x_1_4, x_2_4, x_3_4]   spec of quotient of multivariate polynomial ring
+and exceptional divisor
+  effective cartier divisor defined by
+    sheaf of ideals with restrictions
+      1a: ideal(x_2_1*x_3_1)
+      2a: ideal(x_2_2*x_3_2)
+      3a: ideal(x_3_3)
+      4a: ideal(x_2_4)


This escalated quickly...

I guess the following is better?

julia> b2P3 = blow_up(P3, I2); julia> codomain(b2P3) == P3 true

lkastner · 2023-08-31T10:01:23Z

src/AlgebraicGeometry/ToricVarieties/ToricSchemes/attributes.jl

+  for U in affine_charts(X)
+    for V in affine_charts(X)
+      glue = default_covering(X)[U, V]
+      new_glue = restrict(glue, inverse(iso_dict[U]), inverse(iso_dict[V]), check=true)
+      add_glueing!(cov, new_glue)
+    end
+  end


https://docs.oscar-system.org/dev/DeveloperDocumentation/styleguide/#Code-structure

Thank you for the pointer. Adjusted to

for U in affine_charts(X), V in affine_charts(X) ... end

I tried also

for U,V in affine_charts(X) ... end

But this errored with "invalid iteration specification".

src/AlgebraicGeometry/ToricVarieties/NormalToricVarieties/standard_constructions.jl

…ic variety Co-authored-by: Martin Bies <MBies87@googlemail.com>

…needed

Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

Co-authored-by: Lars Kastner <lkastner@users.noreply.github.com>

…_cone

lkastner

I still wonder what the use case for the forget_* function is. It is a very technical thing and I do not think that users should worry about such a thing. Furthermore I see it more as a constructor, in the same way one can call polyhedral_fan on a toric variety, I would think the name covered_scheme(*) or something similar would be better suited.

HechtiDerLachs · 2023-09-07T14:54:40Z

Thank you for the review @lkastner !

I understand that forget_toric_structure might seem unintuitive. But it was on @HereAround 's wishlist in this specific form, so I did it. Calling it covered_scheme(...), however, is not an option: As of now, an AbstractNormalToricVariety is already an AbsCoveredScheme, so asking for one should return the same object. underlying_scheme on the other hand did not satisfy @HereAround 's requirements.

HereAround · 2023-09-08T09:22:31Z

Thank you for the review @lkastner and approving this complicated PR.

Regarding forget_toric_structure, I do not have a use case just yet. In essence, I asked @HechtiDerLachs in order to (hopefully soon) satisfy my curiosity. As I see it, it should be interesting to compare how the schemes technology operates (e.g. on blowups) without remembering at all that the scheme/variety is/was toric, and then to compare this with the toric procedure. I expect that exactly this might be useful for @apturner to move forward with a sort of abstract blowup, that is yet to be drafted, discussed etc.

Long story short: No use case just yet. However, hopefully soon to be used as inspiration for further developments.

Based on the approval, I assume we can live with this for now. However, should this becomes relevant on a larger scale (i.e. implementing forgetful functors throughout OSCAR), then we can discuss again, adjust the name etc. As per usual, I am happy to adjust the name etc.

HereAround force-pushed the forget_toric_structure branch from 67d5621 to 304eecf Compare August 29, 2023 11:48

HechtiDerLachs commented Aug 29, 2023

View reviewed changes

src/AlgebraicGeometry/ToricVarieties/ToricSchemes/attributes.jl Outdated Show resolved Hide resolved

HereAround force-pushed the forget_toric_structure branch from 304eecf to b2529a8 Compare August 29, 2023 11:57

HereAround mentioned this pull request Aug 29, 2023

[ToricVarieties] More on toric schemes #2740

Closed

HereAround force-pushed the forget_toric_structure branch from b2529a8 to e07e5e3 Compare August 29, 2023 14:04

HereAround marked this pull request as draft August 29, 2023 14:43

HereAround added enhancement New feature or request WIP NOT ready for merging topic: toric schemes labels Aug 29, 2023

HereAround changed the title ~~Forget toric structure~~ [ToricSchemes] Forget toric structure and enhance blow_up method Aug 29, 2023

HereAround force-pushed the forget_toric_structure branch 3 times, most recently from 44961e3 to a3283af Compare August 30, 2023 10:46

HereAround changed the title ~~[ToricSchemes] Forget toric structure and enhance blow_up method~~ [ToricVarieties] Enhance blow_up method Aug 30, 2023

HereAround force-pushed the forget_toric_structure branch from a3283af to 531fc5d Compare August 30, 2023 10:56

HereAround mentioned this pull request Aug 30, 2023

FTheoryTools - Goals for 2024 and beyond #2699

Open

62 tasks

HereAround force-pushed the forget_toric_structure branch 2 times, most recently from 05e7b80 to c6e0faf Compare August 30, 2023 13:11

HereAround removed the WIP NOT ready for merging label Aug 30, 2023

HereAround force-pushed the forget_toric_structure branch from c6e0faf to 08e6c8b Compare August 30, 2023 16:41

HereAround force-pushed the forget_toric_structure branch from 08e6c8b to 0a9bc7d Compare August 30, 2023 20:32

benlorenz mentioned this pull request Aug 31, 2023

docs: stop verbose doctest output after a failure #2749

Merged

benlorenz reviewed Aug 31, 2023

View reviewed changes

experimental/Schemes/IdealSheaves.jl Outdated Show resolved Hide resolved

HereAround marked this pull request as ready for review August 31, 2023 08:52

lkastner requested changes Aug 31, 2023

View reviewed changes

HereAround force-pushed the forget_toric_structure branch from f229bab to e08fe6f Compare August 31, 2023 12:21

HereAround requested a review from lkastner August 31, 2023 15:29

HechtiDerLachs mentioned this pull request Sep 7, 2023

Toric morphisms to morphisms of covered schemes #2779

Merged

HereAround and others added 11 commits September 7, 2023 13:15

[ToricSchemes] Organize method underlying_scheme with helper functions

1ac70bc

[ToricSchemes] Fix typo

6f014f7

[ToricSchemes] Implement forget_toric_structure.

b41e27d

[IdealSheaves] Implement ideal sheaves from ideals in cox ring of tor…

34afacb

…ic variety Co-authored-by: Martin Bies <MBies87@googlemail.com>

[ToricVarieties] Extend blow_up method to use schemes as fallback if …

0f13f56

…needed

Optimize toric ideal sheaves

82bdc0a

Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

[ToricVarieties] Code improvement

09131e5

Co-authored-by: Lars Kastner <lkastner@users.noreply.github.com>

[Schemes] Code improvement

d4535cb

Co-authored-by: Lars Kastner <lkastner@users.noreply.github.com>

[ToricVarieties] Rename dual_cone into weight_cone

85e848f

[ToricVarieties] Add checks to computation of hilbert basis of weight…

d82661e

…_cone

[Schemes] Optimize toric ideal sheaves

75bfdaf

HereAround force-pushed the forget_toric_structure branch from 730c7e0 to 75bfdaf Compare September 7, 2023 11:16

lkastner approved these changes Sep 7, 2023

View reviewed changes

HereAround merged commit 84d3d49 into oscar-system:master Sep 8, 2023
11 of 15 checks passed

	function _generic_blow_up(v::Any, I::Any)
	error("Not yet supported")
	end

[ToricVarieties] Enhance blow_up method #2741

[ToricVarieties] Enhance blow_up method #2741

Conversation

HechtiDerLachs commented Aug 29, 2023

HereAround commented Aug 29, 2023

HereAround commented Aug 30, 2023

HechtiDerLachs commented Aug 30, 2023

HereAround commented Aug 30, 2023

HereAround commented Aug 30, 2023 • edited Loading

benlorenz commented Aug 30, 2023

HereAround commented Aug 30, 2023 • edited Loading

benlorenz commented Aug 30, 2023

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

HereAround commented Aug 31, 2023

lkastner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HereAround Aug 31, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HereAround Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

HereAround Aug 31, 2023 • edited Loading

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

simonbrandhorst Sep 1, 2023 • edited Loading

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

lkastner left a comment

Choose a reason for hiding this comment

HechtiDerLachs commented Sep 7, 2023

HereAround commented Sep 8, 2023

HereAround commented Aug 30, 2023 •

edited

Loading

HereAround commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

HereAround Aug 31, 2023 •

edited

Loading

HereAround Aug 31, 2023 •

edited

Loading

HereAround Aug 31, 2023 •

edited

Loading

simonbrandhorst Sep 1, 2023 •

edited

Loading