Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

questions concerning default rings #932

Closed
micjoswig opened this issue Dec 30, 2021 · 11 comments · Fixed by #1054
Closed

questions concerning default rings #932

micjoswig opened this issue Dec 30, 2021 · 11 comments · Fixed by #1054
Assignees
Labels
Decision question Further information is requested

Comments

@micjoswig
Copy link
Member

function cox_ring(v::AbstractNormalToricVariety)
return get_attribute!(v, :cox_ring) do
Qx, x = PolynomialRing(QQ, :x=>1:rank(torusinvariant_divisor_group(v)))

to be compared with:

function stanley_reisner_ideal(K::SimplicialComplex)
n = nvertices(K)
R, _ = PolynomialRing(ZZ, n)
return stanley_reisner_ideal(R, K)
end

I have several recommendations / questions concerning this code:

  1. I guess we should always allow for an optional ring argument.
  2. Do we want to take QQ or ZZ as a default coefficient ring here? I think ZZ is more natural (and more general, as it can be tensored into whatever).
  3. How do we want to pick the default variable names? Implicitly, as in the simplicial complex code, or explicitly by specifying a vector of variables?
@micjoswig micjoswig added Decision question Further information is requested labels Dec 30, 2021
@micjoswig
Copy link
Member Author

micjoswig commented Dec 30, 2021

We now have two implementations of Stanley-Reisner ideals and rings. Here is a comparsion of the running times (while this is only one example, the behavior is typical):

julia> P = polarize(Polyhedron(Polymake.polytope.rand_sphere(5,60)))
A polyhedron in ambient dimension 5

julia> V = NormalToricVariety(P)
A normal toric variety

julia> @time stanley_reisner_ideal(V);
342.250888 seconds (7.97 M allocations: 689.097 MiB, 0.03% gc time, 0.00% compilation time)

julia> function allrows(I::IncidenceMatrix)
           m,n = size(I)
           return collect( Vector{Int}(Polymake.row(I,k)) for k = 1:m )
       end
allrows (generic function with 1 method)

julia> I = P.pm_polytope.FACETS_THRU_VERTICES;

julia> K = SimplicialComplex(allrows(I))
Abstract simplicial complex of dimension 4 on 60 vertices

julia> @time stanley_reisner_ideal(K);
  0.024923 seconds (49.94 k allocations: 3.336 MiB)

Does the toric implementation go beyond simplicial fans? If not, you probably want to refactor through simplicial complexes.

The two implementations differ as far as the base rings are concerned; see above. Yet this should not matter much for the running times.

@HereAround
Copy link
Member

  1. I guess we should always allow for an optional ring argument.

For clarity - is this optional argument supposed to be QQ or ZZ? Or rather the Cox ring?

If the Cox ring is meant, then providing its grading needs to be considered: We could either accept a non-graded ring and compute a grading (a.k.a. one of the many cokernel projection of the map from the lattice into the torus-invariant divisor group) or - and that happens to be fairly convenient/necessary for my physics applications - provide the graded ring.

  1. Do we want to take QQ or ZZ as a default coefficient ring here? I think ZZ is more natural (and more general, as it can be tensored into whatever).

I am happy with both.

For completeness, let me mention that I asked a similar question in Slack a while back (about October or November). At that time, I was trying to gauge if people would prefer a (formal - if it exists) ring of complex numbers or QQ. At that point, I received suggestions for QQ, which is therefore the current choice for toric varieties.

  1. How do we want to pick the default variable names? Implicitly, as in the simplicial complex code, or explicitly by specifying a vector of variables?

I would think that all of the following cases need to be considered:

  1. Default variable names (e.g. x1, x2, x3,...)
  2. All variable names specified by the user (e.g. u1, u2, w1, w2, w3, ...)
  3. For some special constructions, a more sophisticated choice is needed.

The first special case that comes to my mind are del Pezzo surfaces. It seem standard to label the blowup/exceptional coordinate with e (at least based on my experience/community). Of course, this name could be changed with yet another optional argument - the current choice in toric varieties however is e. Indeed, as part of the caching for toric varieties, I have implemented this so that we now have the following:

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

cox_ring(dP3)
Multivariate Polynomial Ring in 6 variables x[1], e[3], x[2], e[1], ..., e[2] over Rational Field graded by
x[1] -> [1 0 0 0]
e[3] -> [0 1 0 0]
x[2] -> [0 0 1 0]
e[1] -> [0 0 0 1]
x[3] -> [1 1 0 -1]
e[2] -> [-1 0 1 1]

Note that the current variable names in toric varieties are always x[ 1 ]. This is somewhat cumbersome and I was about to prepare a PR to change this to x1 and alike.

A similar situation arises whenever we compute the blowup of a toric variety. Then again, I would find it most convenient to label the new homogeneous variable/exceptional coordinate with e (or e2 if e is already taken...).

Yet another case happens if we compute the Cartesian product of two toric varieties A, B. If the homogeneous coordinates of A are a1, a2, ... and those of B are b1, b2, ... then it would seem natural to have homogeneous coordinate a1, a2,..., b1, b2, ... for A*B (and one can then easily infer the Stanley-Reisner ideal of A*B from those of A and B).

I am not sure how to choose the variable names for A*B if say x1 is both a variable of A and B. Likewise, a choice/convention has to be made if only A has variable names but B has none. (If both have none, then we could fall back to xi).

Of course, there can (and most likely are) other special cases/constructions which require special care.

Does the toric implementation go beyond simplicial fans? If not, you probably want to refactor through simplicial complexes.

@micjoswig It does not go beyond simpicial. So I agree that this should be refactored. In fact - great that your implementation is so much faster! Are there plans for similar implementations of the irrelevant ideal?

@fieker
Copy link
Contributor

fieker commented Jan 3, 2022 via email

@fieker
Copy link
Contributor

fieker commented Jan 3, 2022 via email

@fieker
Copy link
Contributor

fieker commented Jan 3, 2022 via email

@fieker
Copy link
Contributor

fieker commented Jan 3, 2022

Looking at the implementation.... my guess is that the "new" implementation is "just" using Polymake in a suboptimal way. The (a) bottleneck is "primitive_collections" of the "fan". The fast version is using "minimal_nonfaces"

@micjoswig
Copy link
Member Author

The fast version is using "minimal_nonfaces"

Yip, that implementation was tuned quite a bit in the past.

@micjoswig
Copy link
Member Author

  1. I guess we should always allow for an optional ring argument.

For clarity - is this optional argument supposed to be QQ or ZZ? Or rather the Cox ring?

Yes, I meant the Cox ring (which is called "total coordinate ring" in CLS, by the way). Please add that name to the doc, too, with a ref.

If the Cox ring is meant, then providing its grading needs to be considered: We could either accept a non-graded ring and compute a grading (a.k.a. one of the many cokernel projection of the map from the lattice into the torus-invariant divisor group) or - and that happens to be fairly convenient/necessary for my physics applications - provide the graded ring.

Understood. I agree that this should be discussed.

  1. Do we want to take QQ or ZZ as a default coefficient ring here? I think ZZ is more natural (and more general, as it can be tensored into whatever).

I am happy with both.

For completeness, let me mention that I asked a similar question in Slack a while back (about October or November). At that time, I was trying to gauge if people would prefer a (formal - if it exists) ring of complex numbers or QQ. At that point, I received suggestions for QQ, which is therefore the current choice for toric varieties.

This raises an interesting design question. I guess, if you are interested in the cohomology of toric varieties, then QQ is one natural choice. Yet, the ideal always has integer coefficients (for arbitrary simplicial complexes). Typical questions which occur there involve, e.g., Cohen-Macaulayness over some field, and here QQ makes as much sense as any finite field of prime order.

My current implementation for simplicial complexes insists on integer coefficients, but I would prefer that the user can also choose the coefficient ring (together with the variables).

  1. How do we want to pick the default variable names? Implicitly, as in the simplicial complex code, or explicitly by specifying a vector of variables?

I would think that all of the following cases need to be considered:

1. Default variable names (e.g. `x1, x2, x3,...`)

Yip.

2. All variable names specified by the user (e.g. `u1, u2, w1, w2, w3, ...`)

Yip.

3. For some special constructions, a more sophisticated choice is needed.

The first special case that comes to my mind are del Pezzo surfaces. It seem standard to label the blowup/exceptional coordinate with e (at least based on my experience/community). Of course, this name could be changed with yet another optional argument - the current choice in toric varieties however is e. Indeed, as part of the caching for toric varieties, I have implemented this so that we now have the following:

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

cox_ring(dP3)
Multivariate Polynomial Ring in 6 variables x[1], e[3], x[2], e[1], ..., e[2] over Rational Field graded by
x[1] -> [1 0 0 0]
e[3] -> [0 1 0 0]
x[2] -> [0 0 1 0]
e[1] -> [0 0 0 1]
x[3] -> [1 1 0 -1]
e[2] -> [-1 0 1 1]

Note that the current variable names in toric varieties are always x[ 1 ]. This is somewhat cumbersome and I was about to prepare a PR to change this to x1 and alike.

A similar situation arises whenever we compute the blowup of a toric variety. Then again, I would find it most convenient to label the new homogeneous variable/exceptional coordinate with e (or e2 if e is already taken...).

Sure this makes sense. But isn't this a special case of the previous? Where the user specifies some way of naming the variables?

Yet another case happens if we compute the Cartesian product of two toric varieties A, B. If the homogeneous coordinates of A are a1, a2, ... and those of B are b1, b2, ... then it would seem natural to have homogeneous coordinate a1, a2,..., b1, b2, ... for A*B (and one can then easily infer the Stanley-Reisner ideal of A*B from those of A and B).

I am not sure how to choose the variable names for A*B if say x1 is both a variable of A and B. Likewise, a choice/convention has to be made if only A has variable names but B has none. (If both have none, then we could fall back to xi).

Of course, there can (and most likely are) other special cases/constructions which require special care.

Does the toric implementation go beyond simplicial fans? If not, you probably want to refactor through simplicial complexes.

@micjoswig It does not go beyond simpicial. So I agree that this should be refactored. In fact - great that your implementation is so much faster! Are there plans for similar implementations of the irrelevant ideal?

We do not have this yet. But this is much simpler to compute than the Stanley-Reisner ideal. It would make sense to have some technology in OSCAR for dealing with monomials ideals in general.

@micjoswig
Copy link
Member Author

What do you want to do with the ring? QQ[x] is much larger and mostly mathemtically and algorithmically easier, e.g. Groebner in ZZ is a complete pain.

As I sketched above: integer coefficients are the natural choice (for simplicial complexes; one could argue QQ is better for toric varieties), but generally the user should have an option to choose other coefficients.

@micjoswig
Copy link
Member Author

I feel this is a bit too complicated to discuss here. It would be good to have a short session for coding up some sketches together. After the next OSCAR Friday meeting? Some quick sketches, aided by experts on rings in OSCAR, should be enough. Then everybody can fill in some part of the blanks.

HereAround added a commit to HereAround/Oscar.jl that referenced this issue Jan 3, 2022
via methods implemented in polyhedral geometry section

-> Fix for issue reported by @fieker in oscar-system#932
@HereAround
Copy link
Member

I agree that this might be too involved to discuss in large detail here. A first draft towards resolving the points raised (or at least some of them) can be found here: #939

In the current form, this PR is bad/not satisfying (more details in the PR) and should NOT be merged. It should however help the discussion. Hopefully, this can eventually be "the" solution.

HereAround added a commit to HereAround/Oscar.jl that referenced this issue Jan 3, 2022
via methods implemented in polyhedral geometry section

-> Fix for issue reported by @fieker in oscar-system#932
HereAround added a commit to HereAround/Oscar.jl that referenced this issue Jan 4, 2022
via methods implemented in polyhedral geometry section

-> Fix for issue reported by @fieker in oscar-system#932
HereAround added a commit to HereAround/Oscar.jl that referenced this issue Jan 26, 2022
HereAround added a commit to HereAround/Oscar.jl that referenced this issue Jan 26, 2022
HereAround added a commit to HereAround/Oscar.jl that referenced this issue Feb 5, 2022
HereAround added a commit to HereAround/Oscar.jl that referenced this issue Feb 5, 2022
lkastner added a commit that referenced this issue Feb 5, 2022
…ing, names for homogeneous variables and fmpz-valued rays from polymake (#939)

* ToricVarieties: Initialize list of generators with correct type

* ToricVarieties: Change homogeneous variables of Cox ring: x[k] -> xk

* ToricVarieties: Change order of homogeneous variables for del Pezzo surfaces

* ToricVarieties: Ask for user input for blowup_on_ith_torus_orbit

* ToricVarieties: Add set_coordinate_names and coordinate_names

* ToricVarieties: Cox ring uses coordinate_names(v) as indeterminates

* ToricVarieties: Add set_coefficient_ring and coefficient_ring

* ToricVarieties: Cox ring uses coefficient_ring(v) as coefficient ring

* ToricVarieties: Relax construction of projective_space and Hirzebruch_surface
-> Cox ring not set upon construction
-> User can choose coordinate_names and coefficient_ring

* ToricVarieties: Relax construction of del_pezzo surfaces
-> Suggest coordinate names by setting the corresponding attribute
-> cox_ring not set so user can overwrite this choice and set coefficient_ring

* ToricVarieties: toric_ideal uses coefficient_ring

* ToricVarieties: Blowup_on_ith_minimal_torus_orbit uses coefficient_ring

* ToricVarieties: Add test for set_coordinate_names and set_coefficient_ring

* ToricVarieties: Print coefficient ring in Base.show

* ToricVarieties: Use matrix(ZZ, rays(fan(v))) and nrays to process fans of ToricVarieties
-> Fix issue reported by @fieker in #932

* ToricVarieties: Cache polyhedron of ToricVariety upon construction

* ToricVarieties: Compute SR-Ideal via simplicial complex if polytope of variety known

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

* ToricVarieties: Extend documentation on default coefficient_ring and coordinate_names

* ToricVarieties: More efficient simplex computation

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

Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>
Co-authored-by: Lars Kastner <lkastner@users.noreply.github.com>
@lkastner lkastner linked a pull request Feb 5, 2022 that will close this issue
bschroter pushed a commit to bschroter/oscar-matroids that referenced this issue Feb 20, 2022
…ing, names for homogeneous variables and fmpz-valued rays from polymake (oscar-system#939)

* ToricVarieties: Initialize list of generators with correct type

* ToricVarieties: Change homogeneous variables of Cox ring: x[k] -> xk

* ToricVarieties: Change order of homogeneous variables for del Pezzo surfaces

* ToricVarieties: Ask for user input for blowup_on_ith_torus_orbit

* ToricVarieties: Add set_coordinate_names and coordinate_names

* ToricVarieties: Cox ring uses coordinate_names(v) as indeterminates

* ToricVarieties: Add set_coefficient_ring and coefficient_ring

* ToricVarieties: Cox ring uses coefficient_ring(v) as coefficient ring

* ToricVarieties: Relax construction of projective_space and Hirzebruch_surface
-> Cox ring not set upon construction
-> User can choose coordinate_names and coefficient_ring

* ToricVarieties: Relax construction of del_pezzo surfaces
-> Suggest coordinate names by setting the corresponding attribute
-> cox_ring not set so user can overwrite this choice and set coefficient_ring

* ToricVarieties: toric_ideal uses coefficient_ring

* ToricVarieties: Blowup_on_ith_minimal_torus_orbit uses coefficient_ring

* ToricVarieties: Add test for set_coordinate_names and set_coefficient_ring

* ToricVarieties: Print coefficient ring in Base.show

* ToricVarieties: Use matrix(ZZ, rays(fan(v))) and nrays to process fans of ToricVarieties
-> Fix issue reported by @fieker in oscar-system#932

* ToricVarieties: Cache polyhedron of ToricVariety upon construction

* ToricVarieties: Compute SR-Ideal via simplicial complex if polytope of variety known

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

* ToricVarieties: Extend documentation on default coefficient_ring and coordinate_names

* ToricVarieties: More efficient simplex computation

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

Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>
Co-authored-by: Lars Kastner <lkastner@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Decision question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants