Cache computed properties for toric varieties #839

HereAround · 2021-11-29T00:52:44Z

Add two dicts properties and attributes to struct NormalToricVarieties, AffineNormalToricVarieties, ToricDivisors.
Use these dicts to cache computed properties.
Before computation, check if the result is already known/set in these dicts.
Initialize properties for famous toric varieties (projective space, hirzebruch surface and del Pezzo) upon construction.
Improve show methods.
A couple of minor adjustments.

Please note that this PR contains the changes of #838. In this sense it is a draft and must be rebased once this other PR has been merged.

thofma · 2021-11-29T10:01:29Z

There is already a mechanism for this using @declare_other and set_special/get_special, see for example

Oscar.jl/src/Rings/FinField.jl

Lines 26 to 33 in 79d65df

    
           function factored_order(K::FinField) 
        
             l = get_special(K, :factored_order) 
        
             if l === nothing 
        
               l = factor(size(K)-1) 
        
               set_special(K, :factored_order => l) 
        
             end 
        
             return l 
        
           end

For this the structure needs just one additonal declaration, e.g.

Oscar.jl/src/Groups/types.jl

Lines 137 to 140 in d34d698

    
           mutable struct PermGroup <: GAPGroup 
        
              X::GapObj 
        
              deg::Int64       # G < Sym(deg) 
        
              AbstractAlgebra.@declare_other

@fieker rolled this out, so maybe he has an idea on whether we should try to use this everywhere in Oscar.

fieker · 2021-11-29T10:05:33Z

On Mon, Nov 29, 2021 at 02:01:40AM -0800, Tommy Hofmann wrote: There is already a mechanism for this using ***@***.***_other` and `set_special/get_special`, see for example https://github.com/oscar-system/Oscar.jl/blob/79d65df00327fb6009e9c137bbf2bd83e39475b0/src/Rings/FinField.jl#L26-L33 For this the structure needs just one additonal declaration, e.g. https://github.com/oscar-system/Oscar.jl/blob/d34d698db39583b5fd24ca4e3e375a17684fbdd9/src/Groups/types.jl#L137-L140 @fieker rolled this out, so maybe he has an idea on whether we should try to use this everywhere in Oscar.

I thnk it should be used uniformely everywhere - that allows us to improve it also uniformely everywhere (e.g. rename it...)

…

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #839 (comment)

fingolfin · 2021-11-30T11:14:45Z

Yeah, we ought to (a) rename @declare_other and set_special/get_special to something more consistent and sensible, (b) document it and then (c) advertise it to all our devs and beyond

src/ToricVarieties/CyclicQuotientSingularities/CyclicQuotientSingularities.jl

src/ToricVarieties/LineBundles/constructors.jl

fingolfin · 2021-12-02T12:32:48Z

test/ToricVarieties/runtests.jl

@@ -74,7 +74,9 @@ P2 = toric_projective_space(2)
    @test length(irrelevant_ideal(P2).gens) == 3
 end

-H5 = hirzebruch_surface(5)
+rays = [1 0; 0 1; -1 5; 0 -1]


ERROR: LoadError: cannot assign a value to variable Oscar.rays from module Main

test/ToricVarieties/runtests.jl

(*) affine toric varieties (*) normal toric varieties (*) cyclic quotient singuarlties

fingolfin · 2021-12-23T08:51:57Z

src/ToricVarieties/NormalToricVarieties/methods.jl


+    # if no Betti numbers have been computed
+    betti_numbers = Vector{fmpz}


Suggested change

betti_numbers = Vector{fmpz}

This line was intended for type stability, as I wanted to save these numbers always as fmpz (rather than Int, Int64). If removed, I might have to change the following line 19 to betti_numbers = fill(fmpz(-1),2*dim(v)+1), i.e. explicitly invoking the cast of -1 to fmpz(-1) there. (Which is of course ok.)

A related question. As there are several Betti numbers for one variety, it seems reasonable to cache them in a vector. Currently, I fill this vector with -1 at each position. As Betti numbers are always non-negative, a -1 tells that the corresponding Betti number has not (yet) been computed.

But maybe there is a better way than filling with -1s. E.g. to create a vector of suitable length by no entries set and then perform a check if an entry has been set? Suggestions very much appreciated.

This question/issue/challenge repeats for line bundle cohomologies (which I hope to turn to very soon).

This line does not do anything useful: in particular that value is immediately discarded in the following two lines. If you want type stability for this variable, you need to adjust each assignment to it to ensure the right type is assigned, eg via a type assertion and/or an explicit conversion

Thank you. Adjusted in 0113ddb.

fingolfin · 2021-12-23T09:25:27Z

src/ToricVarieties/CyclicQuotientSingularities/CyclicQuotientSingularities.jl

+    if !has_attribute(cqs, :continued_fraction_hirzebruch_jung)
+        set_attribute!(cqs, :continued_fraction_hirzebruch_jung, Vector{fmpz}(pm_object(cqs).CONTINUED_FRACTION))
+    end
+    return get_attribute(cqs, :continued_fraction_hirzebruch_jung)


This and several similar functions could be simplified by using

return get_attribute!(cqs, :continued_fraction_hirzebruch_jung) do return Vector{fmpz}(pm_object(cqs).CONTINUED_FRACTION) end

In general I am thinking about a system to make this stuff even nicer, inspired by GAP but my brain is still mushy

Thank you. That is a good suggestion. Indeed, this change can be done in a ton of places. I will do this in a new commit rather than rebasing everything (unless I hear differently).

Adjusted in d9efae2

fingolfin · 2021-12-23T09:30:17Z

src/ToricVarieties/ToricDivisors/auxilliary.jl

+        end
+    end
+
+    # torusfactor?


?? Doesn't seem to fit the code?

Agreed - I will update this comment (which was intended to say "check what we know about the torusfactor").

NVM: This comment is entirely out of place (copied from the corresponding output string from toric varieties)...

I will adjust this.

Adjusted in 36f51bf.

fingolfin · 2021-12-23T09:33:50Z

src/ToricVarieties/ToricDivisors/auxilliary.jl

+
+    # torusfactor?
+    if last(out_string) == ','
+        out_string = chop(out_string)


Personally I'd write this kind of function differently: first create a Vector{String} and push! all those adjectives into it. Then use join to concatenate cleverly.

Good point. I will adjust accordingly.

36f51bf, a1368a2 have been adjusted accordingly. Let me know what you think.

fingolfin · 2021-12-23T10:02:52Z

src/ToricVarieties/ToricDivisors/constructors.jl

 function Base.show(io::IO, td::ToricDivisor)
-    print(io, "A torus invariant divisor on a normal toric variety")
+    print(io, output_string(td))


why this way? Why not instead print into a string when you really need one?

My thoughts were no deeper than extending the existing code. As always, I am happy to change to a more robust/preferred/quicker alternative. Are saying that print(output_string(td)) is better?

No, I am saying that instead of shoving data into a string and then printing it, it is better to directly print it. And if you sometimes really want a string, you can simply print into a string.

Notw that even join takes an io argument and can thus print instead of creating a (temporary) string

36f51bf, a1368a2 have been adjusted accordingly. Let me know what you think.

Change name of method: ith_betti_number -> betti_number

fingolfin · 2021-12-23T22:59:11Z

src/ToricVarieties/NormalToricVarieties/attributes.jl

+    return get_attribute!(v, :dim) do
+        return pm_object(v).FAN_DIM::Int
+    end


I think some care is needed to retain type stability; the type assertion needs to be moved to the outer return. I think this might work (but didn't test):

Suggested change

return get_attribute!(v, :dim) do

return pm_object(v).FAN_DIM::Int

end

return get_attribute!(v, :dim)::Int do

return pm_object(v).FAN_DIM

end

You can verify if it worked by (a) calling it (to makes sure I got the syntax right, I am not sure), and (b) using @code_warntype on it to verify the return type is inferred correctly.

Thank you for the suggestion @fingolfin . For me, this change leads to the following:

ERROR: LoadError: LoadError: LoadError: syntax: "function" at Oscar.jl/src/ToricVarieties/NormalToricVarieties/attributes.jl:11 expected "end", got "do"

For all it matters, here is a MWE via @code_warntype, which supports your suspicion that type stability requires more care:

v = toric_projective_space(2)
A normal, non-affine, smooth, projective, gorenstein, q-gorenstein, fano, 2-dimensional toric variety without torusfactor

julia> @code_warntype dim(v)
Variables
#self#::Core.Const(AbstractAlgebra.Generic.dim)
v::NormalToricVariety
#1418::Oscar.var"#1418#1419"{NormalToricVariety}

Body::Any
1 ─ %1 = Oscar.:(var"#1418#1419")::Core.Const(Oscar.var"#1418#1419")
│ %2 = Core.typeof(v)::Core.Const(NormalToricVariety)
│ %3 = Core.apply_type(%1, %2)::Core.Const(Oscar.var"#1418#1419"{NormalToricVariety})
│ (#1418 = %new(%3, v))
│ %5 = #1418::Oscar.var"#1418#1419"{NormalToricVariety}
│ %6 = Oscar.get_attribute!(%5, v, :dim)::Any
└── return %6

Hence, if I read this information correctly, Body::Any show that the return type is Any rather than Int.

How about turning function dim(v::AbstractNormalToricVariety) into dim(v::AbstractNormalToricVariety)::Int? This would seem to work and - based on @code_warntype - enusure that the return value is always Int64.

I am uncertain if the failures in the runs of the nightly build (ERROR: LoadError: Failed to precompile CxxWrap) are correlated to the changes in this PR.

writing dim(v::...)::Int has different meaning, though: it converts the result to Int, by inserting a call to convert(Int, ...); while a type assertion does just that, it checks that the value is an Int (if not, an exception is thrown).

Anyway, let's not worry about this for the moment. I describe a potential future solution for this in another comment.

Improve computation of picard group

Rename "rays" to "fan_rays"

fingolfin

Looks good to me.

From my POV we could merge now; the things I mention are truly minor. I'll do it tomorrow or the day after if I hear no complaints

fingolfin · 2021-12-23T23:11:27Z

src/ToricVarieties/NormalToricVarieties/attributes.jl

 function euler_characteristic(v::AbstractNormalToricVariety)
-    f_vector = Vector{Int}(pm_object(v).F_VECTOR)
-    return f_vector[dim(v)]
+    return get_attribute!(v, :euler_characteristic) do
+        f_vector = Vector{Int}(pm_object(v).F_VECTOR)
+        return f_vector[dim(v)]
+    end
 end


Clearly this is a recurring pattern, and one that I'd expect to see more in the future. It would be good to get this closer to what e.g. GAP (and probably also Polymake and other systems) can do in terms of ease of use and capabilities. E.g. having a general way to test if a value is already known. In GAP parlance, for every attribute Attr there is a "tests" HasAttr to detect if the attribute value is known. We could do the same and have methods like has_euler_characteristic (or haseuler_characteristic but I like it more with the underscore?); or perhaps it should be knows_euler_characteristic?

Anyway, a potential macro syntax

@attribute function euler_characteristic(v::AbstractNormalToricVariety) f_vector = Vector{Int}(pm_object(v).F_VECTOR) return f_vector[dim(v)] end

So really the old code just with @attribute prefixed. Also available for inline functions, and it should be engineered to also transfer information about the return type, whether from type assertions or by regular type inference (by using Base.return_types)

Yes that would be great.

fingolfin · 2021-12-28T00:33:58Z

src/ToricVarieties/NormalToricVarieties/attributes.jl

+    return get_attribute!(v, :dim) do
+        return pm_object(v).FAN_DIM::Int
+    end


writing dim(v::...)::Int has different meaning, though: it converts the result to Int, by inserting a call to convert(Int, ...); while a type assertion does just that, it checks that the value is an Int (if not, an exception is thrown).

Anyway, let's not worry about this for the moment. I describe a potential future solution for this in another comment.

src/ToricVarieties/NormalToricVarieties/methods.jl

fingolfin · 2021-12-28T00:41:24Z

src/ToricVarieties/ToricDivisors/attributes.jl

+Return the toric variety of a torus-invariant Weil divisor.
+"""
+function toricvariety(td::ToricDivisor)
+    return get_attribute(td, :toricvariety)


This will return nothing if toricvariety is not set. Is this intentional? Or can it never happen? But if it can never happen, then why is toricvariety even an attribute instead of a regular struct field?

If it really should stay an attribute, at least add a type assertion?

Suggested change

return get_attribute(td, :toricvariety)

return get_attribute(td, :toricvariety)::AbstractNormalToricVariety

You are right - let me turn this into a struct field (it will be set upon construction of every toric divisor td).

HereAround · 2021-12-29T08:31:49Z

Looks good to me.

From my POV we could merge now; the things I mention are truly minor. I'll do it tomorrow or the day after if I hear no complaints

Thank you @fingolfin.

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

HereAround marked this pull request as draft November 29, 2021 01:08

HereAround force-pushed the Cache branch from 3389342 to 5f19d06 Compare November 29, 2021 01:30

fingolfin mentioned this pull request Nov 30, 2021

Rename, document and advertise @declare_other and set_special/get_special #843

Closed

fingolfin reviewed Dec 2, 2021

View reviewed changes

src/ToricVarieties/CyclicQuotientSingularities/CyclicQuotientSingularities.jl Outdated Show resolved Hide resolved

src/ToricVarieties/LineBundles/constructors.jl Outdated Show resolved Hide resolved

fingolfin reviewed Dec 2, 2021

View reviewed changes

test/ToricVarieties/runtests.jl Outdated Show resolved Hide resolved

fingolfin reviewed Dec 2, 2021

View reviewed changes

test/ToricVarieties/runtests.jl Outdated Show resolved Hide resolved

HereAround and others added 7 commits December 21, 2021 11:59

Improve method isprime_divisor

77db348

Remove obsolete code

65d2fe7

Remove obsolete whitespaces

0146254

Apply suggestions from code review

ac1f3a8

Use @attributes mutable struct for

a5a5b73

(*) affine toric varieties (*) normal toric varieties (*) cyclic quotient singuarlties

Cache properties of normal toric varieties

3cce827

Cache attributes of normal toric varieties

3470ef4

HereAround force-pushed the Cache branch 3 times, most recently from 77da1a9 to 4c28072 Compare December 22, 2021 20:38

fingolfin reviewed Dec 23, 2021

View reviewed changes

HereAround force-pushed the Cache branch from 4c28072 to 5a37025 Compare December 23, 2021 22:30

HereAround added 2 commits December 23, 2021 23:36

Cache betti numbers of normal toric varieties

0113ddb

Change name of method: ith_betti_number -> betti_number

Cache attributes of cyclic quotient singularities

95bc6f5

HereAround force-pushed the Cache branch from 5a37025 to d9efae2 Compare December 23, 2021 22:37

fingolfin reviewed Dec 23, 2021

View reviewed changes

HereAround added 4 commits December 24, 2021 00:09

Cache properties upon construction of normal toric variety

a95f3df

Cache properties of toric projective space upon construction

9160c5a

Improve computation of class group and Picard group

185f12d

Add method map_from_cartier_divisor_group_to_picard_group

ba7e75d

Improve computation of picard group

HereAround added 8 commits December 24, 2021 00:09

Cache more attributes of toric projective space upon construction

6084b25

Cache properties of hirzebruch surfaces upon construction

8d271f6

Cache properties of del Pezzo surfaces upon construction

48a68f7

Update tests (otherwise, they test if caching worked upon construction)

35f6339

Cache computed quantities for toric divisors

6d7b636

Sophisticated show method for normal toric varieties

a87dfcd

Sophisticated show method for toric divisors

604e71a

Simplify code for saving/accessing cached information on toric varieties

77aed33

HereAround force-pushed the Cache branch 2 times, most recently from e9e947a to 4fd4a6e Compare December 24, 2021 00:11

Fix bug "cannot assign value to Oscar variable rays"

4f43fa4

Rename "rays" to "fan_rays"

HereAround force-pushed the Cache branch from 4fd4a6e to 4f43fa4 Compare December 24, 2021 00:31

HereAround marked this pull request as ready for review December 24, 2021 01:16

fingolfin approved these changes Dec 28, 2021

View reviewed changes

Improve method betti_number for normal toric variety

aaee3c9

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

fingolfin merged commit 5fa3a5c into oscar-system:master Dec 29, 2021

This was referenced Dec 29, 2021

Improvements toric varieties #929

Merged

Toric Varieties: Completed goals from fall 2021 until December 2022 #763

Closed

HereAround deleted the Cache branch March 7, 2022 19:26


		# if no Betti numbers have been computed
		betti_numbers = Vector{fmpz}

	return get_attribute(td, :toricvariety)
	return get_attribute(td, :toricvariety)::AbstractNormalToricVariety

Cache computed properties for toric varieties #839

Cache computed properties for toric varieties #839

Conversation

HereAround commented Nov 29, 2021 • edited

thofma commented Nov 29, 2021

fieker commented Nov 29, 2021 via email

fingolfin commented Nov 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HereAround Dec 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HereAround Dec 23, 2021 • edited

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 Dec 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HereAround Dec 23, 2021 • edited

Choose a reason for hiding this comment

HereAround Dec 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HereAround Dec 23, 2021 • edited

Choose a reason for hiding this comment

HereAround Dec 23, 2021 • edited

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 Dec 23, 2021 • edited

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 Dec 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin left a 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 Dec 29, 2021 • edited

Choose a reason for hiding this comment

HereAround commented Dec 29, 2021 • edited

HereAround commented Nov 29, 2021 •

edited

fingolfin commented Nov 30, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 23, 2021 •

edited

HereAround Dec 24, 2021 •

edited

HereAround Dec 29, 2021 •

edited

HereAround commented Dec 29, 2021 •

edited