Opened 10 years ago

Closed 10 years ago

#10809 closed enhancement (fixed)

Cartesian products of toric varieties

Reported by: vbraun Owned by: AlexGhitza
Priority: major Milestone: sage-4.7
Component: algebraic geometry Keywords: toric geometry
Cc: novoselt Merged in: sage-4.7.alpha3
Authors: Volker Braun, Andrey Novoseltsev Reviewers: Andrey Novoseltsev, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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

Depends on #10675.

Apply:

  1. trac_10809_fan_cartesian_product.patch
  2. trac_10809_toric_varieties_cartesian_product.patch
  3. trac_10809_reviewer.patch

Attachments (3)

trac_10809_toric_varieties_cartesian_product.patch (4.1 KB) - added by vbraun 10 years ago.
Updated patch
trac_10809_reviewer.patch (13.4 KB) - added by novoselt 10 years ago.
trac_10809_fan_cartesian_product.patch (5.7 KB) - added by vbraun 10 years ago.
Rebased patch

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by vbraun

  • Cc novoselt added
  • Keywords toric geometry added
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to needs_info

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.

comment:3 Changed 10 years ago by vbraun

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.

comment:4 Changed 10 years ago by novoselt

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.

comment:5 Changed 10 years ago by vbraun

  • Status changed from needs_info to needs_review

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.

comment:6 Changed 10 years ago by novoselt

  • Status changed from needs_review to needs_info

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.

Changed 10 years ago by vbraun

Updated patch

comment:7 Changed 10 years ago by vbraun

  • Status changed from needs_info to needs_review

comment:8 Changed 10 years ago by novoselt

  • Authors changed from Volker Braun to Volker Braun, Andrey Novoseltsev
  • Description modified (diff)
  • Reviewers changed from Andrey Novoseltsev to Andrey Novoseltsev, Volker Braun
  • Status changed from needs_review to needs_info

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:

  1. Ray collections can compute their products and the result is used by cones and fans.
  2. Default dual lattices are now ZZ^n, as we have agreed to do on some ticket but it was never done.
  3. 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).
  4. CPR-Fano toric varieties can now handle products with common toric varieties.
  5. 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.

comment:9 Changed 10 years ago by vbraun

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.

comment:10 Changed 10 years ago by novoselt

  • Work issues set to names for product methods

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.

comment:11 Changed 10 years ago by novoselt

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...

Changed 10 years ago by novoselt

comment:12 Changed 10 years ago by novoselt

  • Status changed from needs_info to needs_review
  • Work issues names for product methods deleted

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.

Changed 10 years ago by vbraun

Rebased patch

comment:13 Changed 10 years ago by vbraun

  • Status changed from needs_review to positive_review

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

comment:14 Changed 10 years ago by novoselt

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?

comment:15 Changed 10 years ago by novoselt

  • Description modified (diff)

comment:16 Changed 10 years ago by vbraun

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

comment:17 Changed 10 years ago by novoselt

OK!

comment:18 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:19 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.