Cartesian products of toric varieties

vbraun commented 13 years ago

The two attached patches implement cartesian products for cones/fans and toric varieties, respectively.

Depends on #10675.

Apply:

CC: @novoselt

Component: algebraic geometry

Keywords: toric geometry

Author: Volker Braun, Andrey Novoseltsev

Reviewer: Andrey Novoseltsev, Volker Braun

Merged: sage-4.7.alpha3

Issue created by migration from https://trac.sagemath.org/ticket/10809

vbraun commented 13 years ago

Changed keywords from none to toric geometry

novoselt commented 13 years ago

Reviewer: Andrey Novoseltsev

novoselt commented 13 years ago

comment:2

That's a neat addition, but I don't quite like lattice name decoration and I think it will not look nice with repeated direct sums. Unfortunately, I am not sure what would be better.

One solution is to replace default names with "N3" for a 3-d lattice N, with N^{3} for LaTeX purposes so that N^3_\RR looks nicely. This is actually handy when one works with many lattices of different dimensions, and we can then shorten the current _repr_ of lattices from "3-d lattice N" to just "N3" with cones printed as "2-d cone in N3". I think I am in favour of such a change, the major problem here is that we'll have to fix a lot of doctests, but better now than later ;-)

As a lazier alternative I propose not to change names at all - if a user does not like names "N+N", there is an option to provide custom ones.

vbraun commented 13 years ago

comment:3

I thought about decorating the lattices with their dimension, but often the two lattices have the same dimension (e.g. P1 x P1). Thats why I didn't implement it to begin with.

The current approach really only doesn't work with repeated direct sums of lattices. I don't think that there is any good automated solution in that case, and you'll have to manually set the name or live with ugly ones.

novoselt commented 13 years ago

comment:4

Well, I still think it is better not to decorate names at all. If I see N1+N2 I expect that there is N1 and N2 and their direct sum. Instead, there is only N and its Cartesian square is denoted differently. I mean, people write \ZZ \oplus \ZZ, but not \ZZ_1 \oplus \ZZ_2 with indices added just to distinguish copies. You can already see that one is first and the other is second ;-)

In our case N+N can be a bit misleading if one N is of dimension 2 and the other is of dimension 3, but naming the sum N1+N2 does not make it better. On the other hand, N2+N3 or even N2+N2 would be meaningful, but I don't insist on that.

vbraun commented 13 years ago

comment:5

Ok I've removed the decorations. If both lattices were already called N then I think its fair to say that the user has no particular interest in giving them names, so I'm naming the sum N as well. Thats still nicer than N+N, I think.

novoselt commented 13 years ago

comment:6

In the P2xP2 example I'd say that one should expect the product fan to live exactly in N+N so I think that

        rank = self.rank() + other.rank()
        if self._name==other._name:
            name=None
            dual_name=None
        else:
            name = make_name(self, other, False)
            dual_name = make_name(self.dual(), other.dual(), False)
        if latex(self)==latex(other):
            latex_name=None
            latex_dual_name=None
        else:
            latex_name = make_name(self, other, True)
            latex_dual_name = make_name(self.dual(), other.dual(), True)
        return ToricLattice(rank, name, dual_name, latex_name, latex_dual_name)

can be replaced with

        rank = self.rank() + other.rank()
        name = make_name(self, other, False)
        dual_name = make_name(self.dual(), other.dual(), False)
        latex_name = make_name(self, other, True)
        latex_dual_name = make_name(self.dual(), other.dual(), True)
        return ToricLattice(rank, name, dual_name, latex_name, latex_dual_name)

for simplicity and consistency. Note that if the user makes "lattice N" explicitly, then

sage: myN = ToricLattice(3, "N")
sage: myN.dual()
3-d lattice N*

so if I try to take the sum N + myN, the result has names

N, M, N, M

but N.dual() + myN.dual() will give

M+N*, N+N, M \oplus N^*, N \oplus N

in particular, the sum of duals is not the dual of the sum. With the simpler code that does not try to be intelligent, the first set of names would be

N+N, M+N*, N \oplus N, M \oplus N^*

and the duality will work nicely.

Since duality is important not just for pretty printing, but also for action on lattices, I now strongly feel that the sum should always be called just "left+right".

Of course, it is not difficult to add a few more checks to your code to avoid the above problem. One can also ask if we should identify 4-d N with 2-d N + 2-d N or treat these lattices as different. I guess with my suggestions they will be different, but 2-d N + 2-d N would be the same as 1-d N + 3-d N. I think they all better be different, but that brings us back to the necessity of including ranks into lattice names. Kind of a compromise would be to allow user to provide all names as optional input to direct_sum and if any names were given - pass them to the lattice constructor instead of concatenating existing names. But as a default I still think N+N is the best.

Minor point: it seems that there is no need to import LatexExpr inside the sum function.

vbraun commented 13 years ago

Updated patch

vbraun commented 13 years ago

comment:7

Attachment: trac_10809_toric_varieties_cartesian_product.patch.gz

novoselt commented 13 years ago

Changed author from Volker Braun to Volker Braun, Andrey Novoseltsev

novoselt commented 13 years ago

comment:8

Hi Volker,

Technical note: the patch applies almost fine on top of 4.6.2.rc0+10522+10675 which are positively reviewed. "Almost" disappears if the very first hunk in your patch for cones is removed. Since it is just a white space issue, I propose that you remove it and this ticket becomes next in line after 10675.

I was unifying documentation and optimizing code, so now you need to review the result ;-) Changes that I have made:

Ray collections can compute their products and the result is used by cones and fans.
Default dual lattices are now ZZ^n, as we have agreed to do on some ticket but it was never done.
Fans are now computing their product using "internal representation of fans" rather then products of cones. This should be much faster and more memory efficient then constructing all separate cones and then using them to generate a fan. I have also included optimization for products of complete fans (the product will know that it is complete as well).
CPR-Fano toric varieties can now handle products with common toric varieties.
It is possible to give custom names to variables of the product varieties.

I also thought that Cartesian_product is more in the spirit of other names in our modules, but I have discovered that toric varieties already have CartesianProduct and cartesian_product inherited from general categories framework. I think that it would be confusing to add Cartesian_product as well. I also don't quite feel comfortable overriding cartesian_product with something that has nothing to do with the base method (I mean - they both are Cartesian products, but treated quite differently). So, I am not quite sure what to do... What are your thoughts about it? Did you know about these methods? Maybe new methods should be called just product? Once we settle on something, I will update my patch.

novoselt commented 13 years ago

Description changed:

--- 
+++ 
@@ -1 +1,5 @@
 The two attached patches implement cartesian products for cones/fans and toric varieties, respectively. 
+
+Depends on #10675.
+
+Apply all patches in order.

novoselt commented 13 years ago

Changed reviewer from Andrey Novoseltsev to Andrey Novoseltsev, Volker Braun

vbraun commented 13 years ago

comment:9

I think we should override the base cartesian_product. The category framework provides some default implementations but here it doesn't make much sense to use it.

novoselt commented 13 years ago

comment:10

I've asked on sage-combinat if that's OK: http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/c8dce426c4c5a00f If they are fine, I'll change the names back to cartesian_product. I currently don't need the generic version personally, but I don't want to break things without need.

novoselt commented 13 years ago

Work Issues: names for product methods

novoselt commented 13 years ago

comment:11

Judging from what Nicolas said on sage-combinat-devel, I don't think that we should cover generic cartesian_product.

So as a somewhat dirty but quick solution I propose changing names of new methods to just product.

As a potentially better but longer alternative I think we should

Learn a bit more about current Cartesian products framework.
Implement classes CartesianProductOfToricVarieties(ToricVariety_field, maybe-some-standard-product-class) and CartesianProductOfCPRFanoToricVarieties(CPRFanoToricVariety_field, CartesianProductOfToricVarieties) that will behave like varieties but at the same time will know their product structure and in particular have projection methods.
Make cartesian_product methods return objects of these classes. I think such functionality would be great, but I'd rather leave its implementation to the future...

novoselt commented 13 years ago

comment:12

Attachment: trac_10809_reviewer.patch.gz

OK, names are back to cartesian_product and we will hope to add projection maps later. Feel free to set to it positive review if you are OK with other changes.

novoselt commented 13 years ago

Changed work issues from names for product methods to none

vbraun commented 13 years ago

Rebased patch

vbraun commented 13 years ago

comment:13

Attachment: trac_10809_fan_cartesian_product.patch.gz

Ok good. Applies fine on top of sage-4.7.alpha1

novoselt commented 13 years ago

comment:14

Um, what was wrong with the patch here? My suggestion was to keep it as is and rebase #10140 on top of this one... Does this one still apply after #10675?

novoselt commented 13 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,8 @@

 Depends on #10675.

-Apply all patches in order.
+Apply
+trac_10809_fan_cartesian_product.patch,
+trac_10809_toric_varieties_cartesian_product.patch,
+trac_10809_reviewer.patch
+

vbraun commented 13 years ago

comment:16

I reordered my patches as in #9604 and got some fuzz here, so I rediffed it. My patch queue has now

trac_10675_nef_complete_intersections.patch
trac_10039_parma_polyhedra_library.patch
trac_10943_fan_morphism_non_full_dim_fan.patch
trac_10809_fan_cartesian_product.patch
trac_10809_toric_varieties_cartesian_product.patch
trac_10809_reviewer.patch
trac_10882_kernel_fan.patch
trac_10529_toric_variety_library_names.patch
trac_10529_MPolynomialIdeal_subs.patch
trac_10529_QuotientRingElement_call.patch
trac_10529_SubschemeMorphisms_without_QuotientRing.patch
trac_10529_smoothness_of_algebraic_subschemes.patch
trac_10140_base_cone_on_ppl.patch
trac_10140_fix_toric_variety_doctests.patch
trac_10023_Hilbert_basis.patch
trac_10540_toric_ideals.patch
trac_10540_Spec_of_affine_toric_variety.patch

novoselt commented 13 years ago

comment:17

OK!

jdemeyer commented 13 years ago

Description changed:

--- 
+++ 
@@ -2,8 +2,8 @@

 Depends on #10675.

-Apply
-trac_10809_fan_cartesian_product.patch,
-trac_10809_toric_varieties_cartesian_product.patch,
-trac_10809_reviewer.patch
+**Apply:**
+1. [attachment: trac_10809_fan_cartesian_product.patch](https://github.com/sagemath/sage-prod/files/10652137/trac_10809_fan_cartesian_product.patch.gz)
+2. [attachment: trac_10809_toric_varieties_cartesian_product.patch](https://github.com/sagemath/sage-prod/files/10652135/trac_10809_toric_varieties_cartesian_product.patch.gz)
+3. [attachment: trac_10809_reviewer.patch](https://github.com/sagemath/sage-prod/files/10652136/trac_10809_reviewer.patch.gz)

jdemeyer commented 13 years ago

Merged: sage-4.7.alpha3

sagemath / sage

Cartesian products of toric varieties #10809