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 )
The two attached patches implement cartesian products for cones/fans and toric varieties, respectively.
Depends on #10675.
Apply:
Attachments (3)
Change History (22)
comment:1 Changed 10 years ago by
- Cc novoselt added
- Keywords toric geometry added
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Reviewers set to Andrey Novoseltsev
- Status changed from needs_review to needs_info
comment:3 Changed 10 years ago by
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
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
- 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
- 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.
comment:7 Changed 10 years ago by
- Status changed from needs_info to needs_review
comment:8 Changed 10 years ago by
- 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:
- 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.
comment:9 Changed 10 years ago by
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
- 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
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)
andCartesianProductOfCPRFanoToricVarieties(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
comment:12 Changed 10 years ago by
- 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.
comment:13 Changed 10 years ago by
- 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
comment:15 Changed 10 years ago by
- Description modified (diff)
comment:16 Changed 10 years ago by
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
OK!
comment:18 Changed 10 years ago by
- Description modified (diff)
comment:19 Changed 10 years ago by
- Merged in set to sage-4.7.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
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 thatN^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.