ToricVarieties: Support for immaculate line bundles #2080

HereAround · 2023-03-17T17:49:39Z

No description provided.

fingolfin

Looks good to me from a purely technical POV (I can't comment on the mathematical content).

Thanks!

afkafkafk13 · 2023-03-19T13:50:40Z

src/AlgebraicGeometry/ToricVarieties/cohomCalg/VanishingSets/attributes.jl

-function cohomology_index(tvs::ToricVanishingSet)
-    return tvs.i::Int
+function cohomology_indices(tvs::ToricVanishingSet)
+    return tvs.cis::Vector{T} where {T <: IntegerUnion}


Is the abreviation 'cis' really standard or should we better use a longer/clearer form? (Keeping in mind that not only toric geometers are using Oscar).
I agree, however, that 'cis' is ok as a shorthand for internal use only, if it is well hidden behind a getter function.

This is a purely internal variable, which is never revealed to a user. It is (my) abbreviation for cohomology_indices. When writing this out, the code becomes clunky and hard to read.

Then I am fine with it.

afkafkafk13

Looks good to me, but we should wait for @lkastner to approve it.

src/AlgebraicGeometry/ToricVarieties/cohomCalg/special_attributes.jl

HereAround · 2023-03-20T12:25:11Z

Thank you @benlorenz !

src/AlgebraicGeometry/ToricVarieties/cohomCalg/VanishingSets/attributes.jl

thofma · 2023-03-20T12:35:53Z

Could the field

cis::Vector{T} where {T <: IntegerUnion}

be just

cis::Vector{ZZRingElem}

with the appropriate conversion when initializing? This would be a bit cleaner and friendlier for the compiler, but maybe there is a performance reason to store it as Int if possible?

HereAround · 2023-03-20T12:55:11Z

Could the field
cis::Vector{T} where {T <: IntegerUnion}
be just
cis::Vector{ZZRingElem}
with the appropriate conversion when initializing? This would be a bit cleaner and friendlier for the compiler, but maybe there is a performance reason to store it as Int if possible?

You are right. This indeed looks cleaner. Thank you for the pointer.

As far as I can tell, there is no performance reason to store this as Int.

At least within the toric varieties, IntegerUnion is used frequently. Would you suggest that we change all of them to ZZRingElem?

thofma · 2023-03-20T13:03:31Z

I think we allow IntegerUnion at many places for the convenience of the user and the programmer (for example, in your case when constructing the object), but internally, this will often be stored just as a ZZRingElem.

HereAround · 2023-03-20T13:10:07Z

I think we allow IntegerUnion at many places for the convenience of the user and the programmer (for example, in your case when constructing the object), but internally, this will often be stored just as a ZZRingElem.

Ok. That makes sense.

Hopefully not too far into the future, I shall have the time to take a look at all of the toric varieties and check if and where we could replace IntegerUnion by ZZRingElem. For that reason, I opened a corresponding issue: #2086

lkastner · 2023-03-20T21:31:46Z

I don't really understand what is going on but that might be due to covid. I see that IntegerUnion makes sense in the places it currently exists for the toric varieties. But for the cohomology indices I thought this is like an index in some chain complex. Is that wrong? Do we really expect to perform so well here that we leave the realm of just Int? Until reading this discussion to me these indices were something completely different than say the coefficients of a divisor. And I still think that we don't have to be consistent in all places, i.e. we don't have to use IntegerUnion or fmpz or whichever everywhere. It should be angemessen.

src/AlgebraicGeometry/ToricVarieties/cohomCalg/special_attributes.jl

HereAround · 2023-03-20T22:01:00Z

I don't really understand what is going on but that might be due to covid. I see that IntegerUnion makes sense in the places it currently exists for the toric varieties. But for the cohomology indices I thought this is like an index in some chain complex. Is that wrong? Do we really expect to perform so well here that we leave the realm of just Int? Until reading this discussion to me these indices were something completely different than say the coefficients of a divisor. And I still think that we don't have to be consistent in all places, i.e. we don't have to use IntegerUnion or fmpz or whichever everywhere. It should be angemessen.

I simply did not think long about Int vs. IntegerUnion vs fmpz/did not think it matter that much to people/for performance. But it appears that I was too naive...

My last rebase turned the cohomology indices into type Int.

HereAround · 2023-03-21T10:53:37Z

@lkastner and I are discussing more on this in slack, as there exists similar (if not identical) functionality in Polymake. Hence, back to draft until we have evaluated the situation.

fingolfin · 2023-08-19T09:24:11Z

What's the status of this PR?

HereAround · 2023-08-19T09:32:22Z

There is functionality in Polymake that can compute immaculate bundles on toric spaces with different technology/algorithms. When @lkastner and I last discussed this matter, we agreed that it would be cool to support both variants/approaches. As such, it was agreed that @lkastner would adjust the corresponding Polymake functionality and report back once it is ready.

fingolfin · 2023-09-20T10:06:21Z

@lkastner do you have any estimate when you might have time to get back to this?

Maybe a better (as in: less stressful for you, and overall saving time) approach would be to get this PR ready as-is, with one implementation; and then we can add the second Polymake based implementation whenever @lkastner has time for it?

Co-authored-by: Tommy Hofmann <thofma@gmail.com>

HereAround · 2023-10-09T12:40:33Z

As suggested by @fingolfin, it might be less stressful for @lkastner, if we merge this PR and eventually (whenever this may be) extend it with the corresponding Polymake functionality. Consequently, I have just rebased this PR.

lkastner · 2023-03-20T23:06:02Z

src/AlgebraicGeometry/ToricVarieties/cohomCalg/VanishingSets/attributes.jl

+function cohomology_indices(tvs::ToricVanishingSet)
+    return tvs.cis
 end


Suggested change

function cohomology_indices(tvs::ToricVanishingSet)

return tvs.cis

end

cohomology_indices(tvs::ToricVanishingSet) = tvs.cis

Co-authored-by: Max Horn <max@quendi.de>

HereAround marked this pull request as draft March 17, 2023 17:49

HereAround added enhancement New feature or request topic: toric varieties labels Mar 17, 2023

HereAround force-pushed the immaculate branch 2 times, most recently from 563e167 to d1c9b80 Compare March 17, 2023 19:18

HereAround marked this pull request as ready for review March 17, 2023 22:14

HereAround requested a review from lkastner March 17, 2023 22:14

fingolfin approved these changes Mar 18, 2023

View reviewed changes

afkafkafk13 reviewed Mar 19, 2023

View reviewed changes

benlorenz reviewed Mar 19, 2023

View reviewed changes

src/AlgebraicGeometry/ToricVarieties/cohomCalg/special_attributes.jl Outdated Show resolved Hide resolved

thofma reviewed Mar 20, 2023

View reviewed changes

src/AlgebraicGeometry/ToricVarieties/cohomCalg/VanishingSets/attributes.jl Outdated Show resolved Hide resolved

HereAround mentioned this pull request Mar 20, 2023

Toric Varieties: ZZRingElem vs. IntegerUnion #2086

Closed

fingolfin reviewed Mar 20, 2023

View reviewed changes

src/AlgebraicGeometry/ToricVarieties/cohomCalg/special_attributes.jl Outdated Show resolved Hide resolved

HereAround force-pushed the immaculate branch from a44230d to cbdd33d Compare March 20, 2023 21:57

HereAround marked this pull request as draft March 21, 2023 10:30

HereAround and others added 3 commits October 9, 2023 14:24

ToricVarieties: Support for immaculate line bundles

d410b77

fix jldoctest

1367df2

Improve performance.

b87d4bf

Co-authored-by: Tommy Hofmann <thofma@gmail.com>

HereAround force-pushed the immaculate branch from cbdd33d to 913e832 Compare October 9, 2023 12:38

HereAround marked this pull request as ready for review October 9, 2023 12:38

lkastner approved these changes Oct 9, 2023

View reviewed changes

HereAround force-pushed the immaculate branch from 913e832 to 58aabd2 Compare October 9, 2023 14:39

HereAround enabled auto-merge (rebase) October 9, 2023 15:06

ToricVarieties: Code optimization and bug fixes

515ceb3

Co-authored-by: Max Horn <max@quendi.de>

HereAround force-pushed the immaculate branch from 58aabd2 to 515ceb3 Compare October 9, 2023 16:16

HereAround merged commit 51336ad into oscar-system:master Oct 9, 2023
9 of 12 checks passed

HereAround deleted the immaculate branch October 9, 2023 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ToricVarieties: Support for immaculate line bundles #2080

ToricVarieties: Support for immaculate line bundles #2080

HereAround commented Mar 17, 2023

fingolfin left a comment

afkafkafk13 Mar 19, 2023 •

edited

HereAround Mar 20, 2023 •

edited

afkafkafk13 Mar 20, 2023

HereAround Mar 20, 2023

afkafkafk13 left a comment

HereAround commented Mar 20, 2023

thofma commented Mar 20, 2023 •

edited

HereAround commented Mar 20, 2023 •

edited

thofma commented Mar 20, 2023

HereAround commented Mar 20, 2023

lkastner commented Mar 20, 2023

HereAround commented Mar 20, 2023

HereAround commented Mar 21, 2023 •

edited

fingolfin commented Aug 19, 2023

HereAround commented Aug 19, 2023

fingolfin commented Sep 20, 2023

HereAround commented Oct 9, 2023

lkastner Mar 20, 2023

ToricVarieties: Support for immaculate line bundles #2080

ToricVarieties: Support for immaculate line bundles #2080

Conversation

HereAround commented Mar 17, 2023

fingolfin left a comment

Choose a reason for hiding this comment

afkafkafk13 Mar 19, 2023 • edited

Choose a reason for hiding this comment

HereAround Mar 20, 2023 • edited

Choose a reason for hiding this comment

afkafkafk13 Mar 20, 2023

Choose a reason for hiding this comment

HereAround Mar 20, 2023

Choose a reason for hiding this comment

afkafkafk13 left a comment

Choose a reason for hiding this comment

HereAround commented Mar 20, 2023

thofma commented Mar 20, 2023 • edited

HereAround commented Mar 20, 2023 • edited

thofma commented Mar 20, 2023

HereAround commented Mar 20, 2023

lkastner commented Mar 20, 2023

HereAround commented Mar 20, 2023

HereAround commented Mar 21, 2023 • edited

fingolfin commented Aug 19, 2023

HereAround commented Aug 19, 2023

fingolfin commented Sep 20, 2023

HereAround commented Oct 9, 2023

lkastner Mar 20, 2023

Choose a reason for hiding this comment

afkafkafk13 Mar 19, 2023 •

edited

HereAround Mar 20, 2023 •

edited

thofma commented Mar 20, 2023 •

edited

HereAround commented Mar 20, 2023 •

edited

HereAround commented Mar 21, 2023 •

edited