Improvements toric varieties

[HereAround]

@fingolfin As discussed in #839, a few minor improvements:

• coeffs and toricvariety are now (ordinary) struct fields for toric divisors
• Rename the method betti_number to betti_numbers.

I am not sure how to rewrite the method betti_numbers to use the (standard?) syntax for cached properties. This is a prototype that I expect to repeat for line/vector bundle cohomologies very soon. So it would seem worth to think more about this.

1. The user can provide any integer i and thereby ask for i-th Betti number with i<0 or i>2 * dimension(variety). In this case, one could either raise an error ("this Betti number is not defined") or return 0. Currently, the latter is implemented, but on second thought, I would think it is more reasonable to opt for the error.
   Irrespective, this test should probably be conducted before checking if the attribute betti_numbers is known and has a properly initialized value at position i+1.
2. Next, check if betti_numbers has a meaningful/initialized value at position i+1. If yes, return it and otherwise compute it.
3. Currently, I initialize the vector betti_numbers with values -1. Since this number is never a Betti number (as Betti numbers are dimensions of a vector spaces), I can then proceed as follows:

if (betti_numbers[i+1] == -1) ... #compute it...

In summary, I am unsure how to adjust

return get_attribute!(v, :betti_numbers) do ...

such that it returns only the i+1st element of the vector betti_numbers if this i+1st element has been initialized (i.e. is not -1 in the current implementation).

Feedback/suggestions very much appreciated.

[fingolfin]

That renaming was not what I had in mind at all (though you are free to do it if you prefer). What I meant was that the vector with Betti numbers could be accessed via a helper with that name; since it is internal, perhaps prefix it by an underscore, so _betti_numbers

[HereAround]

Ok. Let me remove the commit with the changes on betti_number then.

I am uncertain if I understand your suggestion (both regarding the helper and get_attribute!, in the message below). Could you please elaborate more?

[fingolfin]

By the way, the above few lines could be rewritten using get_attribute!, too. Instead of adding a _betti_nimbers helper

[HereAround]

@fingolfin I have just updated this PR, so that there are no changes to betti_number were made other than

return betti_numbers[i+1]

as you suggested above. Could you please reiterate in more detail what changes you propose for this method? (If your time permits, maybe you could even push a proposed change?) Thank you!

[fingolfin]

I've pushed a commit with changes to the Betti number code, please check it and feel free to edit/revert/whatever

[fingolfin]

By the way, you can observe the effects of these various changes. With master, before this PR, we get this:

julia> P2 = NormalToricVariety(normal_fan(Oscar.simplex(2)))
A normal toric variety

julia> Base.return_types(betti_number)
1-element Vector{Any}:
 Any

So it basically could not make any guess as to what the return type might be. You can get more information with @code_warntype, which I recommend to use in the terminal, as the coloring there is quite helpful. Looking at that, we immediately see other optimization opportunities, e.g. it can't infer the return type of dim(v) (but we already know that, and as I said, I am working on a patch to improve this)

julia> @code_warntype betti_number(P2, 2)
Variables
  #self#::Core.Const(Oscar.betti_number)
  v::NormalToricVariety
  i::Int64
  #1481::Oscar.var"#1481#1482"{NormalToricVariety, _A, Int64} where _A
  f_vector::Any
  k::Int64
  betti_numbers::Any

Body::Any
1 ──       Core.NewvarNode(:(#1481))
│          Core.NewvarNode(:(f_vector))
│          Core.NewvarNode(:(k))
│          Core.NewvarNode(:(betti_numbers))
│    %5  = Oscar.dim(v)::Any
│    %6  = (2 * %5)::Any
│    %7  = (i > %6)::Any
└───       goto #3 if not %7
2 ──       goto #4
3 ── %10 = (i < 0)::Bool
└───       goto #5 if not %10
4 ┄─       return 0
5 ── %13 = Oscar.has_attribute(v, :betti_number)::Bool
│    %14 = !%13::Bool
└───       goto #7 if not %14
6 ── %16 = Oscar.fmpz(-1)::fmpz
│    %17 = Oscar.dim(v)::Any
│    %18 = (2 * %17)::Any
│    %19 = (%18 + 1)::Any
│          (betti_numbers = Oscar.fill(%16, %19))
└───       goto #8
7 ── %22 = Oscar.get_attribute(v, :betti_number)::Any
│    %23 = Core.apply_type(Oscar.Vector, Oscar.fmpz)::Core.Const(Vector{fmpz})
└───       (betti_numbers = Core.typeassert(%22, %23))
8 ┄─ %25 = betti_numbers::Any
│    %26 = (i + 1)::Int64
│    %27 = Base.getindex(%25, %26)::Any
│    %28 = (%27 == -1)::Any
└───       goto #13 if not %28
9 ── %30 = Oscar.isodd(i)::Bool
└───       goto #11 if not %30
10 ─ %32 = Oscar.fmpz(0)::fmpz
│    %33 = betti_numbers::Any
│    %34 = (i + 1)::Int64
│          Base.setindex!(%33, %32, %34)
└───       goto #12
11 ─       (k = Oscar.div(i, 2))
│    %38 = Core.apply_type(Oscar.Vector, Oscar.Int)::Core.Const(Vector{Int64})
│    %39 = Oscar.pm_object(v)::Polymake.BigObject
│    %40 = Base.getproperty(%39, :F_VECTOR)::Any
│          (f_vector = (%38)(%40))
│          Oscar.pushfirst!(f_vector, 1)
│    %43 = Oscar.:(var"#1481#1482")::Core.Const(Oscar.var"#1481#1482")
│    %44 = Core.typeof(v)::Core.Const(NormalToricVariety)
│    %45 = Core.typeof(f_vector)::DataType
│    %46 = Core.typeof(k)::Core.Const(Int64)
│    %47 = Core.apply_type(%43, %44, %45, %46)::Type{Oscar.var"#1481#1482"{NormalToricVariety, _A, Int64}} where _A
│    %48 = f_vector::Any
│          (#1481 = %new(%47, v, %48, k))
│    %50 = #1481::Oscar.var"#1481#1482"{NormalToricVariety, _A, Int64} where _A
│    %51 = k::Int64
│    %52 = Oscar.dim(v)::Any
│    %53 = (%51:%52)::Any
│    %54 = Base.Generator(%50, %53)::Base.Generator{_A, _B} where {_A, _B}
│    %55 = Oscar.sum(%54)::Any
│    %56 = Oscar.fmpz(%55)::Any
│    %57 = betti_numbers::Any
│    %58 = (i + 1)::Int64
└───       Base.setindex!(%57, %56, %58)
12 ┄       Oscar.set_attribute!(v, :betti_number, betti_numbers)
13 ┄ %61 = Oscar.get_attribute(v, :betti_number)::Any
│    %62 = (i + 1)::Int64
│    %63 = Base.getindex(%61, %62)::Any
└───       return %63

With this PR, before my commit:

julia> Base.return_types(betti_number)
1-element Vector{Any}:
 Any

With my commit:

julia> Base.return_types(betti_number)
1-element Vector{Any}:
 fmpz

BTW I've now added another commit which deals with a few more issues:

• calling dim(v) repeatedly is inefficient
• the return type of Vector{Int}(pm_object(v).F_VECTOR) can not be inferred because that of pm_object(v).F_VECTOR can't be inferred (and constructors can in principle return anything!). So instead use convert(Vector{Int}, pm_object(v).F_VECTOR) which also inserts a type assertion. In fact I used an implicit convert call which looks like this: f_vector::Vector{Int} = pm_object(v).F_VECTOR

[fingolfin]

I squashed my two commits and fixed an off-by-one-bug 😂

[HereAround]

Thank you @fingolfin . This looks great! I am very happy with these changes.

[HereAround]

I like that the method now immediately returns 0 when i is odd. As you said - reduces the amount of memory used for storage and circumvents a lot of unnecessary computations. Great!

[HereAround]

Nice. This is the part that I was confused by before. Thank you!

[HereAround]

@fingolfin I am happy with this PR and do not plan to make further changes. If you are happy, please consider merging this. Thank you.

[HereAround]

I have to correct myself. Since I cached Betti numbers for "standard spaces", I had to update those number to reflect that we do no longer store the trivial numbers for odd i. Other than that, this should be good to go (if the tests are successful).