Sage: Ticket #9972: Add fan morphisms
https://trac.sagemath.org/ticket/9972
<p>
This ticket adds a module for fan morphisms - morphisms between lattices with specified fans in the domain and codomain, which are compatible with this morphism. Compatibility check and automatic construction of the codomain fan or refinement of the domain fan are implemented.
</p>
<p>
Patch order (applies cleanly to sage-4.6.rc0):
</p>
<ol><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9972/trac_9972_add_cone_embedding.patch" title="Attachment 'trac_9972_add_cone_embedding.patch' in Ticket #9972">trac_9972_add_cone_embedding.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9972/trac_9972_add_cone_embedding.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9972/trac_9972_improve_element_constructors.patch" title="Attachment 'trac_9972_improve_element_constructors.patch' in Ticket #9972">trac_9972_improve_element_constructors.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9972/trac_9972_improve_element_constructors.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9972/trac_9972_remove_enhanced_cones_and_fans.patch" title="Attachment 'trac_9972_remove_enhanced_cones_and_fans.patch' in Ticket #9972">trac_9972_remove_enhanced_cones_and_fans.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9972/trac_9972_remove_enhanced_cones_and_fans.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9972/trac_9972_add_fan_morphisms.patch" title="Attachment 'trac_9972_add_fan_morphisms.patch' in Ticket #9972">trac_9972_add_fan_morphisms.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9972/trac_9972_add_fan_morphisms.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9972/trac_9972_fix_fan_warning.patch" title="Attachment 'trac_9972_fix_fan_warning.patch' in Ticket #9972">trac_9972_fix_fan_warning.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9972/trac_9972_fix_fan_warning.patch" title="Download"></a>
</li></ol><p>
See <a class="closed ticket" href="https://trac.sagemath.org/ticket/9604" title="task: Tracker bug for toric varieties (closed: invalid)">#9604</a> for dependencies.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/9972
Trac 1.1.6novoseltThu, 23 Sep 2010 02:07:19 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_add_toric_lattice_morphisms.patch</em>
</li>
</ul>
TicketnovoseltThu, 23 Sep 2010 02:19:05 GMTstatus changed; cc set
https://trac.sagemath.org/ticket/9972#comment:1
https://trac.sagemath.org/ticket/9972#comment:1
<ul>
<li><strong>cc</strong>
<em>vbraun</em> added
</li>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_info</em>
</li>
</ul>
<p>
The patch is in principle ready, but while we are at it - do we want to make custom <code>_repr_</code> for such morphisms? If yes, how should they be different from the standard?
</p>
<p>
Also, the speed is far from spectacular, but it is not easy to make it better until simple polyhedra work faster - currently most time is spend on constructing them for intersection purposes and I tried hard not to intersect more cones than necessary.
</p>
TicketvbraunMon, 27 Sep 2010 20:38:58 GMTreviewer set
https://trac.sagemath.org/ticket/9972#comment:2
https://trac.sagemath.org/ticket/9972#comment:2
<ul>
<li><strong>reviewer</strong>
set to <em>Volker Braun</em>
</li>
</ul>
<p>
I don't have any good suggestion for how to improve <code>_repr_</code>, we can always leave that for later.
</p>
<p>
I'll rewrite the Polyhedron constructor in cython one of these days, that should fix the speed issues. Though its a good idea to minimize the number of intersections computed :)
</p>
<p>
The current version looks good for an initial shot at toric morphisms. Are you still changing things around or should I officially review it?
</p>
TicketnovoseltMon, 27 Sep 2010 23:58:49 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:3
https://trac.sagemath.org/ticket/9972#comment:3
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
I don't plan on changing these functions further, so this ticket is ready for review!
</p>
TicketvbraunMon, 04 Oct 2010 10:52:43 GMT
https://trac.sagemath.org/ticket/9972#comment:4
https://trac.sagemath.org/ticket/9972#comment:4
<p>
I like the functionality, but I'm confused about the name. Is this supposed to be a <code>ToricLatticeMorphism</code> or a <code>FanMorphism</code>? I am thinking that it would be good to split those apart, and perhaps make the latter inherit from the former.
</p>
<p>
<code>ToricLatticeMorphism.make_compatible_with(fan)</code> doesn't make the morphism compatible with the fan, it is the other way round. So it should be either <code>fan.make_compatible_with(toric_morphism)</code> or, say, <code>ToricLatticeMorphism.subdivide_domain(domain_fan,codomain_fan)</code>. Or see below.
</p>
<p>
Another functionality that I would like to have is to figure out the image fan from the lattice morphism and the domain. How about the following proposal:
</p>
<ol><li>separate <code>ToricLatticeMorphism</code> and <code>FanMorphism</code>.
</li></ol><ol start="2"><li><code>FanMorphism(lattice_hom, domain=Fan, codomain=Fan)</code> constructs the fan morphism. If <code>lattice_hom</code> is a matrix the corresponding <code>ToricLatticeMorphism</code> is constructed automatically. Raises <code>ValueError</code> if the fans are not compatible.
</li></ol><ol start="3"><li><code>FanMorphism(lattice_hom, domain=Fan)</code> constructs the image fan and uses it as codomain. Raise <code>ValueError</code> if not possible.
</li></ol><ol start="4"><li><code>FanMorphism(lattice_hom, domain=Fan, codomain=Fan, subdivide=True)</code> will subdivide the domain fan as necessary.
</li></ol><p>
Let me know what you think & I'd be happy to help, of course!
</p>
TicketnovoseltMon, 04 Oct 2010 16:22:37 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:5
https://trac.sagemath.org/ticket/9972#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
I am now putting finishing touches on plotting, but then return back to morphisms.
</p>
<p>
After actually working on morphisms a bit, I had second thoughts about class organization. In particular, I didn't see how <code>FanMorphism</code> can fit nicely into Sage and I didn't even understand what it is mathematically. A fan is a collection of cones with certain restrictions, right? Then a fan morphism should be a map between sets of cones, in our case finite. But that's not what we want, we rather want a morphism between supports of fans, which is given on points of the space by a linear map. Put it another way, we want a <code>ToricLatticeMorphism</code> restricted to the support of the domain. During construction of such a morphism we have to check that everything is compatible, which can be quite expensive. During any application of this morphism to a point, we should check that this point is in the domain and this is also non-trivial (the only thing that will work for general fans is checking this point against every generating cone). So if we do have a special class for <code>FanMorphism</code>, how about this definition: "it is a morphism between toric lattices with distinguished fans in the domain and codomain which are compatible with this morphism." I.e. the domain is still a whole toric lattice and there is no need for complicated checks. One can write then
</p>
<pre class="wiki">sage: phi = FanMorphism(lattice_morphism, F1, F2)
sage: phi.image_cone(cone_of_F1)
<some cone of F2>
sage: phi.preimage_cones(cone_of_F2)
<a tuple of cones of F1>
</pre><p>
Going further, I don't think anymore that we should derive special fans for domain/codomain of morphisms between toric varieties themselves, let's call this class <code>ToricMorphism</code>. The problem I have is that a single variety can be a (co)domain of different morphisms. (I definitely need such functionality.) This can make it very inconvenient to work with divisors and classes because they will get confused which parent do they belong to, we will have to work with coercion, and the code may need to look something like this:
</p>
<pre class="wiki">sage: fan = ...
sage: X = ToricVariety(fan)
sage: fan = X.fan() # This is already a bit strange...
sage: phi = ToricMorphism(matrix(...), X, Y)
sage: psi = ToricMorphism(matrix(...), Z, X)
sage: X_fan_codomain = psi.codomain().fan() # These two lines are plain confusing
sage: X_fan_domain = phi.domain().fan()
sage: phi.domain().rational_divisor_group() == psi.codomain().rational_divisor_group()
???
</pre><p>
All four fans above are mathematically the same, the only difference is what kind of extra functionality do they get. But they will be different Python objects and associated toric varieties will be also different objects for no apparent reason, i.e. how these reasons can be explained to a user rather than a developer?
</p>
<p>
So I think that either morphisms derive their own fans for domain/codomain and use them internally without actually changing the varieties they were created for (i.e. <code>phi.domain().fan()</code> is the same as <code>X.fan()</code>, but <code>phi.domain_fan()</code> can be something specialized), or they store this information in some other way and return (pre)images taking cones of usual fans as arguments.
</p>
<p>
I recall that we already had a similar argument in the beginning, whether or not we need any kind of specialized cones, which provide clean access to new features. I just checked how many new features got added:
</p>
<ul><li>TWO methods for <code>Cone_of_fan</code>: <code>star_generators</code> and <code>star_generator_indices</code>. (There were actually many new methods here originally, but others migrated to plain cones.) Still, I think that this class is justified, because these are very natural operations to perform on cones that belong to a fan. Also, a cone cannot quite belong to several fans in the sense that its internal data structures are severely tied to a fan and it is very important performance-wise. (E.g. intersection of cones of the same fan is incomparably easier/faster than for arbitrary ones, and this will remain true even when polyhedra are made fast.)
</li><li>ONE method for <code>Cone_of_toric_variety</code>: <code>cohomology_class</code>. Here I feel less convinced that it is necessary, <code>cone.cohomology_class()</code> does not feel more natural to me than <code>X.cohomology_class(cone)</code>. However, I think there are more methods added here by patches which are not applied in my queue and something else may come up during later development. If not, we should probably reconsider this class, because it would be nice to have
<pre class="wiki">sage: ToricVariety(fan).fan() is fan
True
</pre></li><li>Hypothetical cones of (co)domain would each add one more method, but make it difficult/inconvenient to deal with multiple morphisms, while the whole point of making new classes should be making life easier...
</li></ul><p>
For this ticket I propose the following:
</p>
<ol><li>Rename <code>make_compatible_with</code> to <code>subdivide_domain</code>.
</li><li>Add to <code>ToricLatticeMorphism</code> a method like <code>image_fan(domain_fan)</code> to construct the "natural" fan in the codomain, as you have suggested.
</li><li>Add <code>FanMorphism(ToricLatticeMorphism)</code> which mathematically is what I have said above, will live in the same Hom-space as lattice morphisms, and will behave in the same way (including <code>domain</code> and <code>codomain</code> returning toric lattices), except that it also has <code>domain_fan</code> and <code>codomain_fan</code> methods returning fans which are guaranteed to be compatible with the morphism. It also has <code>image_cone</code> and <code>preimage_cones</code>, results are cached (perhaps it is more efficient to compute them at once for all cones or even do it during compatibility check).
</li><li>Cones of fans also get <code>image_cone</code> and <code>preimage_cones</code> methods that take as an argument a <code>FanMorphism</code> with appropriate fans.
</li></ol><p>
A follow-up ticket will add <code>ToricMorphism</code> for arbitrary scheme morphisms between toric varieties and <code>ToricEquivariantMorphism</code> for those coming from <code>FanMorphisms</code>.
</p>
<p>
Let me know what you think!
</p>
TicketvbraunMon, 04 Oct 2010 17:56:10 GMT
https://trac.sagemath.org/ticket/9972#comment:6
https://trac.sagemath.org/ticket/9972#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9972#comment:5" title="Comment 5">novoselt</a>:
</p>
<blockquote class="citation">
<p>
"it is a morphism between toric lattices with distinguished fans in the domain and codomain which are compatible with this morphism."
</p>
</blockquote>
<p>
Yes, that is the usual definition. No restriction on the support of the underlying lattice map.
</p>
<p>
On an unrelated note, I would call "point" = "0-dimensional torus orbit" = full-dimensional cone. A toric morphism maps points to potentially higher-dimensional torus orbits. Though I do understand that you meant lattice points.
</p>
<p>
I understand your issue about having multiple morphisms. But if the fans don't know about the toric morphism then they shouldn't know about the toric variety either, otherwise its confusing. So in principle I don't mind getting rid of the <code>Cone_of_toric_variety</code> class. At least this solves the dilemma <code>cone_of_variety.cohomology_class()</code> vs. <code>variety.cohomology_class(cone)</code>; since the cone doesn't know about the variety only the latter can work. But instead of adding a <code>cohomology_class</code> method, I'd rather have the <code>CohomologyRing</code> element constructor accept cones:
</p>
<pre class="wiki"> sage: HH = X.cohomology_ring()
sage: HH( X.fan(2)[23] )
</pre><p>
This pattern works already for the divisor group and Chow group:
</p>
<pre class="wiki">sage: Div = X.divisor_group()
sage: Div( X.fan(1)[0] ) # output should have been V(z0)?!?
V(1-d cone of Rational polyhedral fan in 2-d lattice N)
sage: A = X.Chow_group()
sage: A( X.fan(1)[0] )
The Chow cycle (1, 0, 0)
</pre><blockquote class="citation">
<p>
For this ticket I propose the following:
</p>
<ol><li>Rename <code>make_compatible_with</code> to <code>subdivide_domain</code>.
</li><li>Add to <code>ToricLatticeMorphism</code> a method like <code>image_fan(domain_fan)</code> to construct the "natural" fan in the codomain, as you have suggested.
</li></ol></blockquote>
<p>
If we can agree on a hierarchy <code>ToricLatticeMorphism</code> -> <code>FanMorphism</code> -> <code>ToricMorphism</code> then <code>ToricLatticeMorphism</code> shouldn't know about fans at all, if only to avoid circular imports. Similar to how <code>ToricLattice</code> doesn't know about <code>Fan</code>. So <code>make_compatible_with</code> and <code>image_fan</code> become special cases of the <code>FanMorphism</code> constructor.
</p>
<p>
And instead of an <code>is_compatible_with</code> method, why not have <code>FanMorphism(matrix,fan1,fan2)</code> raise <code>ValueError, "Cone <3,4,5> is not contained in any image cone"</code>.
</p>
<blockquote class="citation">
<ol start="3"><li>Add <code>FanMorphism(ToricLatticeMorphism)</code> which mathematically is what I have said above, will live in the same Hom-space as lattice morphisms, and will behave in the same way (including <code>domain</code> and <code>codomain</code> returning toric lattices), except that it also has <code>domain_fan</code> and <code>codomain_fan</code> methods returning fans which are guaranteed to be compatible with the morphism. It also has <code>image_cone</code> and <code>preimage_cones</code>, results are cached (perhaps it is more efficient to compute them at once for all cones or even do it during compatibility check).
</li></ol></blockquote>
<p>
Yes, sounds good!
</p>
<blockquote class="citation">
<ol start="4"><li>Cones of fans also get <code>image_cone</code> and <code>preimage_cones</code> methods that take as an argument a <code>FanMorphism</code> with appropriate fans.
</li></ol></blockquote>
<p>
I'd rather have only <code>morphism(cone)</code> (or <code>morphism.image(cone)</code>) and <code>morphism.preimages(cone)</code>.
</p>
<blockquote class="citation">
<p>
A follow-up ticket will add <code>ToricMorphism</code> for arbitrary scheme morphisms between toric varieties and <code>ToricEquivariantMorphism</code> for those coming from <code>FanMorphisms</code>.
</p>
</blockquote>
<p>
I agree, but can we use, say, <code>AlgebraicMorphism</code> and <code>ToricMorphism</code>? Toric should really always be replaceable with "torus-equivariant".
</p>
TicketnovoseltMon, 04 Oct 2010 18:26:06 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/9972#comment:7
https://trac.sagemath.org/ticket/9972#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>switch to FanMorphism</em>
</li>
</ul>
<p>
OK, I wanted to avoid "compatibility check by exception" but I can live with it ;-) How about this:
</p>
<ol><li>Move <code>cone.cohomology_class</code> functionality to the element constructor of cohomology ring (I actually think this is the best and the most clear way there can be.) Can you perhaps make a patch for this change since it involves mostly your code?
</li><li>Get rid of <code>Cone_of_toric_variety</code> (that's my part so I can do this). This raises a question whether <code>EnhancedCone/Fan</code> should survive at all. One option is to put <code>_</code> in front so that they disappear from the documentation.
</li><li>Make <code>FanMorphism</code> work as you have described, including informative error message.
</li><li>See if there is then any point in having <code>ToricLatticeMorphism</code> at all.
</li><li>No changes to cones, all (pre)images are computed/stored by morphisms. Which is probably also the cleanest way to do it.
</li></ol><p>
I am also OK with <code>ToricMorphism</code> for equivariant ones. I'll see what name should fit nicely with existing classes for affine/projective morphisms for a "non-toric morphism between toric varieties."
</p>
TicketvbraunMon, 04 Oct 2010 18:37:22 GMT
https://trac.sagemath.org/ticket/9972#comment:8
https://trac.sagemath.org/ticket/9972#comment:8
<ol><li>I'll write a patch and post it here for the <code>cohomology ring</code> and <code>divisor_group</code>.
</li></ol><ol start="2"><li>I don't see why we need the <code>Enhanced*</code> versions, then.
</li></ol>
TicketvbraunMon, 04 Oct 2010 21:24:50 GMT
https://trac.sagemath.org/ticket/9972#comment:9
https://trac.sagemath.org/ticket/9972#comment:9
<p>
I'll also tighten <code>x in fan</code> to only return <code>True</code> if <code>x</code> is actually a <code>cone</code>. I'll add another method <code>fan.support_contains(point)</code> for the other usage. That'll make it easier to ensure that "something" is a cone of the correct fan. Otherwise we have stuff like <code>0 in fan</code> return True...
</p>
TicketnovoseltTue, 05 Oct 2010 02:23:14 GMT
https://trac.sagemath.org/ticket/9972#comment:10
https://trac.sagemath.org/ticket/9972#comment:10
<p>
Sounds good!
</p>
TicketvbraunTue, 05 Oct 2010 17:08:50 GMT
https://trac.sagemath.org/ticket/9972#comment:11
https://trac.sagemath.org/ticket/9972#comment:11
<p>
Patch is attached. I removed 'cohomology_class' from <code>Cone_of_toric_variety</code> to make sure that I got all occurrences, but tried to make as few other changes in that area as possible. Can you try to put my patch at the bottom of this ticket's queue?
</p>
<p>
I added an overriding method <code>Cone_of_fan.is_equivalent</code> (see the "TODO" comment) that you should uncomment after removing the enhanced cones.
</p>
<p>
The <code>CohomologyRing._element_constructor_</code> now accepts cones as well. For the other issue, you should have complained that <code>divisor_group</code> returns the non-toric divisor group and only <code>toric_divisor_group</code> should accept cones ;-) The latter already works as it should.
</p>
TicketnovoseltFri, 08 Oct 2010 01:04:40 GMT
https://trac.sagemath.org/ticket/9972#comment:12
https://trac.sagemath.org/ticket/9972#comment:12
<p>
Regarding <code>Cone_of_fan.is_equivalent</code> - cones of fan are also constructed by intersection method and during computing the cone lattice. They can certainly be non-unique now and I don't think that we should try to make them unique. So I propose removal of the overriding method.
</p>
TicketvbraunFri, 08 Oct 2010 11:01:49 GMT
https://trac.sagemath.org/ticket/9972#comment:13
https://trac.sagemath.org/ticket/9972#comment:13
<p>
My thought was that it can be expensive to ensure that two cones of a fan are not the same, and comparing ambient ray indices would be faster. And you always have to go through all cones of the fan to test membership, so it will be called often. I thought about whether that is a problem during the construction, but then I found it easiest to leave that question up to you ;-)
</p>
<p>
How about constructing <code>Cone_of_fan</code> always though a factory function that tests (via <code>Cone.is_equivalent</code>) if the cone is already in the fan. That would be simple to implement and we can then rely on uniqueness of the <code>Cone_of_fan</code>.
</p>
TicketnovoseltFri, 08 Oct 2010 14:13:15 GMT
https://trac.sagemath.org/ticket/9972#comment:14
https://trac.sagemath.org/ticket/9972#comment:14
<p>
What membership exactly do you want to test?
</p>
<p>
The assumption is that there are no invalid cones of the fan, so if you want
to check if a cone is in the fan, it is enough to check if the ambient fan of the cone is the fan in question. For checking equivalence of two cones of the same fan it is enough indeed to check that their ambient ray indices are the same, as tuples, since they are always assumed to be sorted and when they are not - it is a bug.
</p>
<p>
I don't mind adding uniqueness of cones of fan or even cones themselves for that matter (on a separate ticket, perhaps?), I just don't quite understand why do you want it. The advantages that I see are
</p>
<ul><li>memory saving;
</li><li>cached data sharing;
</li></ul><p>
and disadvantages
</p>
<ul><li>more complicated code for construction;
</li><li>longer time for construction.
</li></ul><p>
Do you have something else in mind?
</p>
TicketnovoseltFri, 08 Oct 2010 15:45:06 GMT
https://trac.sagemath.org/ticket/9972#comment:15
https://trac.sagemath.org/ticket/9972#comment:15
<p>
Some more thoughts:
</p>
<ol><li>When there is an attempt to check if a point is in the fan, should we try to interpret this point as a ray, i.e. 1-d cone?
</li><li>The last line of <code>_contains</code> should be replaced with the original version:
<pre class="wiki">try:
return data.is_equivalent(self.cone_containing(data.rays()))
except ValueError: # No cone containing all these rays
return False
</pre>For example, if you take a fan which is a subdivision of a cone, then this cone will trickle down to this block, but <code>cone_containing</code> will raise an exception. There probably should be a test for such a situation (although I certainly don't claim that all other places test all possible exceptions...)
</li><li><code>Fan.contains</code> should no longer accept many arguments after your change - let's replace <code>*arg</code> with <code>cone</code>.
</li><li>Typo: "or a something" should be without "a".
</li><li>Typo: "class associated to cones" should be "classes".
</li></ol><p>
Otherwise looks great: the new approach allows users to create their own shortcuts and write <code>HH(cone)</code> instead of <code>cone.cohomology_class()</code>. While I like names exposed in Sage to be descriptive, I certainly don't want to force users do it in their code ;-)
</p>
<p>
I will base other patches of the ticket on top of this one.
</p>
TicketvbraunFri, 08 Oct 2010 17:01:17 GMT
https://trac.sagemath.org/ticket/9972#comment:16
https://trac.sagemath.org/ticket/9972#comment:16
<p>
A fan is a collection of cones. A point is never in a fan, as it is not a cone. I don't think we should make cones unique in general, only cones of a fan. You can have arbitrarily many cones (memory permitting), but the cones of a fan are a finite set; To me it then feels right to have the <code>Cone_of_fan</code> be unique. In the current implementation they actually are unique after the fan is constructed. So it ends up being a minor change to guarantee that they are always unique, I think.
</p>
TicketvbraunFri, 08 Oct 2010 18:51:54 GMT
https://trac.sagemath.org/ticket/9972#comment:17
https://trac.sagemath.org/ticket/9972#comment:17
<p>
One potential danger is that <code>cone.ambient_ray_indices()</code> is meaningless if <code>cone</code> is only mathematically equivalent to a fan, but not a <code>Cone_of_fan</code>. I've added a new method <code>RationalPolyhedralFan.get_cone(cone)</code> that finds the <code>Cone_of_fan</code> corresponding to the cone, and I tried to make sure it gets called wherever we accept arbitrary cones from the user.
</p>
<p>
But its a potential pitfall to watch out for. We could always insist on the user passing only <code>Cone_of_fan</code>, but that seems to be too unwieldy for the user. I would suggest that we move the <code>ambient_ray_indices()</code> accessor method to <code>Cone_of_fan</code> and convert the code in <code>cone.py</code>, <code>fan.py</code> to use the data member <code>_ambient_ray_indices</code> instead. All higher level functions then shall only use <code>cone.ambient_ray_indices()</code>, converting a generic cone to a <code>Cone_of_fan</code> via <code>fan.get_cone(cone)</code> if necessary.
</p>
TicketvbraunFri, 08 Oct 2010 18:57:58 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_improve_element_constructors.2.patch</em>
</li>
</ul>
<p>
Updated patch
</p>
TicketnovoseltFri, 08 Oct 2010 20:43:45 GMT
https://trac.sagemath.org/ticket/9972#comment:18
https://trac.sagemath.org/ticket/9972#comment:18
<p>
In what sense <code>ambient_ray_indices</code> is dangerous? If ambient structures of two cones are different, there is no point to look at these attributes at all. Otherwise they are the same if and only if cones are the same.
</p>
<p>
I am definitely against moving <code>ambient_ray_indices</code> to <code>Cone_of_fan</code> because the point in having it is uniform treatment of faces of cones and cones of fans, which are pretty much the same things. In fact, even <code>star_generators</code> make sense for faces of a cone in the sense of containing facets, it just should not be called that name. So currently the main reason for a <code>Cone_of_fan</code> to exist is that terminology for faces of cones and cones of fans is a bit different. I think that it should stay this way as much as possible, so that all cones behave the same.
</p>
<p>
I am also against new containment check - cones are equal if they have the same rays in the same order and equivalent if they define the same set of points. If the same cone happened to belong to different fans and so has two objects representing it, it does not change anything. We can check if cones belong to the same ambient structure for code optimization, but the output should be the same. Note that in this case this check actually can be done in generic cones and there is no need to override the method for <code>Cone_of_fan</code>.
</p>
<p>
I still don't understand exactly what are you trying to achieve in general and in particular why do we need <code>get_cone</code> method. I agree that <code>fan.contains</code> should return <code>True</code> only for (some) cones and not for anything else, because a fan is a collection of cones. I agree that it may be good to have uniqueness of <code>Cone_of_fan</code> but I don't see any reasons for doing this except for some performance gain and it is not clear how significant it can be. It also seems to me that it makes more sense to make all cones unique based on the ordered rays and the ambient structure, because essentially this is how cones of fans can be made unique.
</p>
TicketnovoseltFri, 08 Oct 2010 20:53:04 GMT
https://trac.sagemath.org/ticket/9972#comment:19
https://trac.sagemath.org/ticket/9972#comment:19
<p>
So for <code>is_equivalent</code> optimization I propose inserting
</p>
<pre class="wiki"> if self == other:
return True
if self.ambient() == other.ambient():
return self.ambient_ray_indices() == other.ambient_ray_indices()
</pre><p>
in the beginning of <code>Cone.is_equivalent</code>. Maybe with <code>==</code> replaced with <code>is</code> in the <code>if</code> statements - both variants will work correctly, the question is how many simple but potentially non-informative checks we want to perform before using a generic algorithm.
</p>
TicketvbraunSat, 09 Oct 2010 14:13:42 GMT
https://trac.sagemath.org/ticket/9972#comment:20
https://trac.sagemath.org/ticket/9972#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9972#comment:19" title="Comment 19">novoselt</a>:
</p>
<blockquote class="citation">
<p>
In what sense ambient_ray_indices is dangerous? If ambient structures of two cones are different, there is no point to look at these attributes at all.
</p>
</blockquote>
<p>
Thats of course true, but we got that wrong in the <code>ToricDivisor</code> constructor (no check that the ambient was the same) and you didn't catch it ;-)
</p>
<p>
I'm just trying to explore options that make it impossible to make this mistake in the future. One more idea would be to force the user to pass the ambient to <code>ambient_ray_indices()</code>,
</p>
<pre class="wiki">sage: fan1 = ...
sage: fan2 = ...
sage: fan1.generating_cone(2).ambient_ray_indices(fan1) # fast
sage: fan1.generating_cone(2).ambient_ray_indices(fan2) # slower
</pre><p>
This would also get rid of the need for <code>get_cone()</code>
</p>
<p>
The point of the <code>get_cone</code> method is to avoid this extra branch whenever the user passes a cone that is not necessarilly within the same ambient:
</p>
<pre class="wiki"> if is_Cone(c):
if c.ambient()==fan
indices = c.ambient_ray_indices()
else:
try:
indices = tuple(sorted([ fan.rays().index(r) for r in c.rays() ]))
except ValueError:
...
</pre><p>
and instead just write
</p>
<pre class="wiki"> if is_Cone(c):
indices = fan.get_cone(c).ambient_ray_indices()
</pre>
TicketnovoseltSat, 09 Oct 2010 14:57:38 GMT
https://trac.sagemath.org/ticket/9972#comment:21
https://trac.sagemath.org/ticket/9972#comment:21
<p>
Aha, now I see! I really don't like the idea of forced arguments to <code>ambiet_ray_indices</code> and <code>get_cone</code> seems to be a confusing name. How about this:
</p>
<ol><li>Implement <code>get_cone</code> functionality using <code>__call__</code> for fans and cones, i.e. you will have to write <code>fan(c).ambient_ray_indices()</code> to make sure that indices are correct. The same goes for other relative things like face walking methonds - in all cases it is assumed that your cone knows where does it sit. This goes very well with the concept of fans being collections of cones - you "convert" a certain cone to an element of this collection. For cones it is not as transparent but still makes perfect sense, IMHO.
</li><li>In principle, I don't terribly mind adding *optional* argument so that one can write <code>c.ambient_ray_indices(fan)</code> and internally it will be translated to <code>fan(self).ambient_ray_indices()</code>. However, I then want to have it for all functions where it does make sense, it will add the same piece of code and documentation to all of them, and in the user code it does not lead to any significant clarification or space saving. So I'd rather not add such functionality and you don't usually like having two different ways to do the same thing ;-)
</li><li>Leave equivalence and containment checks mathematical, i.e. it is possible to get <code>True</code> for a check that a cone of one fan is "in" another fan. Those who want to check if it is an actual element of the collection should use <code>cone.ambient() is fan</code>.
</li></ol><p>
If you agree with these proposals, then I can start implementing them and uniqueness of cones (and then fans too for uniformity, I guess) working on top of your patch.
</p>
TicketvbraunSat, 09 Oct 2010 15:46:09 GMT
https://trac.sagemath.org/ticket/9972#comment:22
https://trac.sagemath.org/ticket/9972#comment:22
<p>
Adding the functionality to <code>__call__</code> still does not enforce that <code>ambient_ray_indices</code> is used correctly. In particular, it would not have caught the bug in <code>ToricDivisor</code>.
</p>
<p>
How about we remove the <code>ambient_ray_indices()</code> method from cones altogether and replace it with <code>fan.ray_indices(cone)</code>.
</p>
<p>
I agree with 3.
</p>
<p>
I don't think that the <code>Cone_of_fan</code> uniqueness is particularly urgent, we can come back to that later.
</p>
TicketnovoseltSat, 09 Oct 2010 16:41:01 GMT
https://trac.sagemath.org/ticket/9972#comment:23
https://trac.sagemath.org/ticket/9972#comment:23
<p>
It is not quite Python style to try to eliminate any possibility of user mistakes by making sure that everything is called properly in proper places. The assumption is that users know what they are doing ;-) These methods assume that your cone is in some fixed fan:
</p>
<ul><li><code>adjacent</code>
</li><li><code>ambient_ray_indices</code>
</li><li><code>face_lattice</code> (indirectly - since elements will be cones of the same fan)
</li><li><code>faces</code> (indirectly)
</li><li><code>facet_of</code>
</li><li><code>facets</code> (indirectly)
</li><li><code>star_generator_indices</code>
</li><li><code>star_generators</code>
</li></ul><p>
All of these methods are pretty important and convenient, in particular, in many cases <code>ambient_ray_indices</code> is more useful than <code>rays</code> or <code>ray_matrix</code> since it allows you to see clearly how different cones are related to each other. It would be quite annoying if for using all these methods it was necessary to mention fan explicitly.
</p>
<p>
Yes, there are bugs that appear because users make wrong assumptions, but I don't think that it is a valid reason to require users to always explicitly state all of their assumptions so that each function can check that they are correct and when possible fix them.
</p>
<p>
In addition, as you pointed out in a sample code above, passing ambient explicitly will mean that it always works correctly, but sometimes fast and sometimes slow. This sometimes slow can be sometimes significantly slow and the reason for keeping track of these ray indices and ambients is precisely code optimization, otherwise all cones could store only their rays and all fans - only their cones.
</p>
<p>
So I still think that if you have a cone and it is important that things about this cone are computed relevant to a certain fan, you are responsible for making sure that this cone "sits" in this fan and if not - trying to convert it there. I see a point in making this conversion easy, but not in making it mandatory every time you need it. Compare this
</p>
<pre class="wiki">sage: indices = fan.ray_indices(cone)
sage: supercones = fan.cones_with_facet(cone)
</pre><p>
and this
</p>
<pre class="wiki">sage: indices = fan(cone).ambient_ray_incides()
sage: supercones = fan(cone).facet_of()
</pre><p>
I think in the second case it is quite clear that
</p>
<pre class="wiki">sage: cone = fan(cone)
sage: indices = cone.ambient_ray_incides()
sage: supercones = cone.facet_of()
</pre><p>
is likely to be faster, plus it is a bit easier to read and there is only one place where an exception can occur due to <code>cone</code> being incompatible with <code>fan</code> at all.
</p>
<p>
I also think that using cones from a wrong fan is likely to be exposed very quickly in actual work due to index-out-of range exceptions, so given several month of working with current interface, I really don't want it to change...
</p>
TicketvbraunSat, 09 Oct 2010 18:24:25 GMT
https://trac.sagemath.org/ticket/9972#comment:24
https://trac.sagemath.org/ticket/9972#comment:24
<p>
Well <code>face_lattice</code>, <code>faces</code>, <code>facet_of</code>, and <code>facets</code> should return mathematically equivalent results if the cone is in the wrong ambient, so they don't count. For <code>adjacent</code> and <code>star_generators</code> I would tend to let your argument pass that the output would be so wildly wrong that it is immediately obvious. But <code>ambient_ray_indices</code> of a <code>Cone</code> always returns valid index ranges (namely, <code>range(0,cone.nrays())</code>).
</p>
<p>
So I do maintain that this method is particularly dangerous. Note that I don't want to get rid of the <code>_ambient_ray_indices</code> data member, just
</p>
<pre class="wiki">def ray_indices(self, cone):
if self is cone.ambient():
return cone._ambient_ray_indices
# slow fallback
</pre>
TicketnovoseltSat, 09 Oct 2010 19:17:00 GMT
https://trac.sagemath.org/ticket/9972#comment:25
https://trac.sagemath.org/ticket/9972#comment:25
<p>
Well, if you just want to add <code>fan.ray_indices(cone)</code> method (which can be potentially slow), I am OK with it, although I'd rather not ;-) But I really want to keep <code>cone.ambient_ray_indices()</code> as is without any arguments, especially forced ones.
</p>
<p>
And I still think that the best way is to explicitly convert input cones to cones of a particular fan and then freely use any fan-dependent methods. Given the current size of toric code it does not seem to me that we have any particularly dangerous methods (the most dangerous are <code>check=False</code> and <code>normalize=False</code> options, which are described as dangerous). So maybe instead of changing code we can put a warning in the documentation that this method may be dangerous and one should ensure that <code>cone.ambient()</code> is what it should be before calling this method. It seems to me that it is quite difficult to start using <code>ambient_ray_indices</code> without reading its documentation at least once.
</p>
TicketvbraunSat, 09 Oct 2010 20:00:53 GMT
https://trac.sagemath.org/ticket/9972#comment:26
https://trac.sagemath.org/ticket/9972#comment:26
<p>
I agree that the best way is to first convert the input cone to the fan and then work with that.
</p>
<p>
But 1) its easy to accidentally omit that step and 2) it is very hard to detect that you got it wrong. In my book, thats a very dangerous API design. I don't think a warning in the documentation is sufficient here, after all, we should have known better yet we fell into that trap with <code>ToricDivisor</code>.
</p>
<p>
At the very least <code>ambient_ray_indices()</code> should spit out a warning if <code>self==self.ambient()</code> [what ARE you trying to do?]. But the best option seems to me to be the <code>fan.ray_indices(cone)</code> method. If you wrote your code correctly there won't be any slowdown, and if you forgot to convert the cone to a cone of the fan then you'll still get the right answer. Ideally we'd then make <code>ambient_ray_indices</code> private not use it outside of <code>cone.py</code>/<code>fan.py</code> (and fan morphisms).
</p>
TicketnovoseltSun, 10 Oct 2010 03:08:43 GMT
https://trac.sagemath.org/ticket/9972#comment:27
https://trac.sagemath.org/ticket/9972#comment:27
<p>
After some more contemplating and checking
</p>
<pre class="wiki">$ grep "ambient_ray_indices()" sage/geometry/* |wc
30 179 2579
$ grep "ambient_ray_indices()" sage/schemes/generic/* |wc
13 85 1383
</pre><p>
on Sage-4.6.alpha3, I think that you may be a bit overestimating the danger. We have used this function in 43 places and so far everything is working quite well. The "mistake" you ran into got quickly caught even before the ticket got ready for a review. Also, <em>why</em> did you get this mistake? Because you took a code from a place where the ambient definitely was set correctly and moved it to the place where potentially other cones maybe passed in. Finally, <em>you</em> actually have not done any mistake, you just left a possibility for a user to make one by passing a wrong cone. (In this case, I think, the mistake would be realized quite fast even if the code did not throw up an exception.)
</p>
<p>
Adding a warning for calling <code>ambient_ray_indices</code> on the ambient itself may not work because it may be called internally in some cycles. (I didn't check but it is quite likely.) Besides, nothing is wrong with such a call. I also strongly believe that this method must be open/documented/public, because if we have used it in 43 places, it is quite important for development and therefore users who build their own structures on top of stuff shipped in Sage are likely to find it convenient in their code. I personally use it all the time when working with varieties and sometimes even regret that these indices are not the default output for cones, so when I have a list of them I need to write a cycle calling this method.
</p>
<p>
A few months ago I already suggested switching to notation like <code>fan.ray_indices(cone)</code> etc. but you opposed and pointed out that it is not in the spirit of OOP. Now I actually completely agree and think that it is very fortunate that we have not done so then. I even still see some benefits of having special classes for cones of toric varieties and morphisms (in particular, this problem would not occur ;-)) but for these cases the disadvantages overweight.
</p>
<p>
Have I convinced you yet or should I abandon any hope?-)
</p>
TicketvbraunSun, 10 Oct 2010 10:31:08 GMT
https://trac.sagemath.org/ticket/9972#comment:28
https://trac.sagemath.org/ticket/9972#comment:28
<p>
I had counted the occurrences of <code>ambient_ray_lattice</code> as well, but my conclusion was that it is seldomly used outside of <code>/sage/geometry/cone,fan.py</code>. Replacing the 13 occurrences wouldn't be hard. The fact that copy/pasting "correct" code turns it so easily into "incorrect" code is precisely what I find disturbing. Also, the mistake in <code>ToricDivisor</code> was not caught easily. It is in the current release. And saying that its the user's fault ("You are holding it wrong" :-) is not helpful.
</p>
TicketnovoseltSun, 10 Oct 2010 14:28:31 GMT
https://trac.sagemath.org/ticket/9972#comment:29
https://trac.sagemath.org/ticket/9972#comment:29
<p>
I meant that mistake would be realized quite fast once the code was actually used. I didn't actually use code for toric divisors yet, especially with cones from another fan. But anyway - this is all hypothetical and I may be very wrong.
</p>
<p>
My main point is not that it is difficult to replace 13 uses of <code>ambient_ray_indices</code>, but that it is a very convenient and natural function which I personally use both when developing new code and when actually using Sage interactively. I would rather not pass any arguments to it, because it is annoying for interaction and in functions it may lead to code like
</p>
<pre class="wiki">cone.ambient_ray_indices(cone.ambient())
</pre><p>
or
</p>
<pre class="wiki">cone.ambient().ray_indices(cone)
</pre><p>
which is plain confusing.
</p>
<p>
The name of the function clearly indicates that there is something ambient and cone must be aware of it, since no arguments were passed. Therefore, when using this function, the user should be sure that this something ambient is what it is supposed to be. In fact, it may be more clear for new outside users than for us, since we are used to writing code inside the class where in many cases this requirement is definitely satisfied. (How can <code>self</code> have a wrong <code>self.ambient()</code>???) And after all this discussion I will probably remember very well for a long time to make an extra check that this function is used after making sure that ambient is what it is assumed to be.
</p>
<p>
If you want to add extra functionality like <code>fan.ray_indices(cone)</code> which will be safer to use and then use this one when you want - I am fine with it. But if you insist on getting rid of <code>cone.ambient_ray_indices()</code> in its present form/namespace, we need to invite more people from sage-devel for opinions...
</p>
TicketnovoseltSun, 10 Oct 2010 14:48:22 GMT
https://trac.sagemath.org/ticket/9972#comment:30
https://trac.sagemath.org/ticket/9972#comment:30
<p>
By the way, in schemes <code>ambient_ray_indices</code> is used several times in the examples and it was also used in Hal's examples for their book. It is very-very natural and should be as easy to access as possible...
</p>
TicketvbraunSun, 10 Oct 2010 15:43:50 GMT
https://trac.sagemath.org/ticket/9972#comment:31
https://trac.sagemath.org/ticket/9972#comment:31
<p>
<code>ambient_ray_indices</code> is not natural! Natural operations on cones should work without referring to a particular enumeration of the rays. Think <code>cone1.intesection(cone2)</code> vs. <code>set(cone1.ambient_ray_indices()).intersection(set(cone2.ambient_ray_indices()))</code>. Of course sometimes you can't avoid it, definitely not in the internal implementations, but higher-level code (like the toric varieties) could easily move away from it. Case in point is that it is used only 9 times (the other 4 are in doctests).
</p>
<p>
And this looks horrible
</p>
<pre class="wiki">cone.ambient().ray_indices(cone)
</pre><p>
precisely because it is bad! It will break things! Its like a big, flashing sign: Do not write this code! You really want to use <code>self.fan().ray_indices(cone)</code>!
</p>
<p>
I don't mind the availability of <code>cone.ambient_ray_indices()</code> so much if you use it on the command line; In that case you know what your ambient() is. But I think we should ban its use from the toric varieties code. Then we algorithmically audit it for correctness by grepping <code>sage/schemes/generic</code> for <code>ambient_ray_indices</code>. How does that sound?
</p>
TicketnovoseltMon, 11 Oct 2010 00:52:26 GMT
https://trac.sagemath.org/ticket/9972#comment:32
https://trac.sagemath.org/ticket/9972#comment:32
<p>
Well, instead of "very-very" let me say that <code>ambient_ray_incices</code> are as natural as coordinates. Coordinates may be not very convenient for definitions, since you need to make sure that the choice of the coordinate system does not matter. They are in many cases not mathematically natural in the sense that there are many different choices without any preference. At the same time they are quite useful both for proofs and for working with concrete examples.
</p>
<p>
Now, choosing between <code>cone.ambient_ray_indices()</code> and <code>fan.ray_indices(cone)</code> is like choosing between always mentioning which coordinate system you are using whenever you use coordinates and having some coordinate system "understood from context." Of course, in the second case you are risking to encounter a situation when it was not quite clear and it leads to mistakes since coordinates in one system may have very little to do with coordinates in another one. People do such mistakes from time to time, that's just life. But it is very convenient to have this default and I still vote for it despite the mistake that you have discovered. (By the way - how did you find it?)
</p>
<p>
Leaving a function but banning it for internal use is a bit silly - the way to enforce it is to scan for occurrences of this function and then replace it with a "safe version". If you do it, then the second step can be making sure that it is used safely and correctly.
</p>
TicketnovoseltMon, 11 Oct 2010 17:34:34 GMT
https://trac.sagemath.org/ticket/9972#comment:33
https://trac.sagemath.org/ticket/9972#comment:33
<p>
In an effort to be less stubborn, here is what I think I should do if we cannot live with the existing interface:
</p>
<ol><li>Keep internal fields <code>_ambient</code> and <code>_ambient_ray_indices</code> which can be related either to a cone or a fan (from the coding point of view these situations are similar and it is very convenient to treat them together - I know for sure since my initial version didn't do it).
</li><li>Remove <code>ambient()</code> and <code>ambient_ray_indices()</code> methods. In the code we still can use attributes since they always must be set in the constructor. Also remove <code>adjacent</code> and <code>facet_of</code> methods from cones and <code>star_generator_indices</code> and <code>star_generators</code> from cones of fan. The class <code>Cone_of_fan</code> becomes completely unnecessary and should be discarded.
</li><li>Add methods corresponding to the above ones with respect to an explicit ambient (of course, there is no analog for <code>ambient</code> itself anymore):
<pre class="wiki">sage: ambient_cone.ray_indices(cone)
sage: ambient_cone.adjacent_faces(cone)
sage: ambient_cone.faces_with_facet(cone)
sage: fan.ray_indices(cone)
sage: fan.adjacent_cones(cone)
sage: fan.cones_with_facet(cone)
sage: fan.star_generators(cone)
sage: fan.star_generator_indices(cone)
</pre>Cones still will cache all this stuff for their own <code>_ambient</code> in a hope that it will be useful later and that's how usually they will be called.
</li></ol><p>
But I still need some voting before performing such a change.
</p>
<p>
I still think that it is convenient to have a default for "the thing before dot" above. This opinion is based on actually working with such structures, so maybe I have bad habits, but that's what I prefer. I also still think that it is natural in mathematics. E.g. Fulton on p.52 defines Star(tau), where it is assumed that tau is a cone in some fan. In a lot of papers using reflexive polytopes people introduce some notation for dual faces. I have never seen such a notation mentioning the ambient polytope, yet the notion of a dual face does not make sense without one. One can argue that a face is supposed to know its ambient, because it is a face and not just a standalone polytope. But with this line of thoughts I think toric divisors may require a cone of an appropriate fan as an input. Or we can agree that we will use any input cone and try to interpret it as a cone in this appropriate fan, in which case it is our responsibility to ensure that we do such an interpretation correctly. It is also common to fix some index set and then use it to index rays of the fan, their generators, corresponding homogeneous variables, and divisors. So I still think that these indices are relevant beyond the implementation guts of cones...
</p>
TicketvbraunTue, 12 Oct 2010 12:40:47 GMT
https://trac.sagemath.org/ticket/9972#comment:34
https://trac.sagemath.org/ticket/9972#comment:34
<p>
I don't want to rewrite the entire interface, and I think that having some implicit assumption about what the <code>ambient()</code> is is usually fine. Also, 3.) is spectacularly ugly :-P As I said before, if the method call e.g. returns again a collection of cones then you'll immediately notice that you had the wrong <code>ambient()</code>. The difference with <code>ambient_ray_indices</code> is that its output will fail in much more subtle ways if the hidden assumption is wrong.
</p>
<p>
If you desperately want to keep <code>ambient_ray_indices()</code>, how about we prefix any use in the toric varieties code with an assertion that makes it explicit. This would be yet another way to make the implicit assumption explicit and have it easily machine-verifiable.
</p>
<p>
On an unrelated note, I don't like <code>cone_of_fan = fan(cone)</code>, its too similar to <code>Fan([cone])</code>. How about <code>Fan.cone_equivalent_to(cone)</code>, see also the already-existing similar method <code>Fan.cone_containing(cone)</code>.
</p>
TicketnovoseltWed, 13 Oct 2010 04:54:22 GMT
https://trac.sagemath.org/ticket/9972#comment:35
https://trac.sagemath.org/ticket/9972#comment:35
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9972#comment:34" title="Comment 34">vbraun</a>:
</p>
<blockquote class="citation">
<p>
3.) is spectacularly ugly :-P
</p>
</blockquote>
<p>
I wholeheartedly agree ;-)
</p>
<blockquote class="citation">
<p>
If you desperately want to keep <code>ambient_ray_indices()</code>, how about we prefix any use in the toric varieties code with an assertion that makes it explicit. This would be yet another way to make the implicit assumption explicit and have it easily machine-verifiable.
</p>
</blockquote>
<p>
I desperately want to keep it! I think that assertions are great - I don't use them much mostly because I didn't know about them until reviewing some of your code, but I am totally for them. However using them always before <code>ambient_ray_indices()</code> is too harsh - I went through <code>cone.py, fan.py, toric_variety.py, fano_toric_variety.py</code> and in all cases it was actually already clear that <code>cone._ambient</code> is correct, e.g. because <code>cone</code> was constructed in the same code. So I think that more generally we should use assertions for input parameters, which was the problem in this case.
</p>
<blockquote class="citation">
<p>
On an unrelated note, I don't like <code>cone_of_fan = fan(cone)</code>, its too similar to <code>Fan([cone])</code>. How about <code>Fan.cone_equivalent_to(cone)</code>, see also the already-existing similar method <code>Fan.cone_containing(cone)</code>.
</p>
</blockquote>
<p>
<code>fan(cone)</code> is similar to <code>Fan([cone])</code> only if your fan is called <code>fan</code> ;-) I was thinking of such format to mimic Element-Parent behaviour in Sage, but:
</p>
<ul><li>currently we don't use this model for cones and fans;
</li><li>if we did, then cones must be both elements (of fans) and parents (of points), and this is not yet supported in Sage;
</li><li>I don't clearly see advantages of such a switch;
</li><li>I would like to have the same interface for "putting" cones into fans and faces into cones.
</li></ul><p>
So the attached patch implements for cones and fans methods <code>embed(cone)</code>. <code>cone_equivalent_to</code> seems unclear to me despite of being long, but I am not sure if <code>embed</code> is much better - what do you think of it? Maybe <code>embed_cone/embed_face</code> would be more clear?
</p>
<p>
I propose a bit different implementation of this method than in your patch. For fans it computes potential ray indices and then uses <code>cone_containing</code> method. This should be faster than using <code>cone_containing</code> directly on the rays and does not trigger cone lattice computation. For cones it will go through all faces of the relevant dimension, but instead of checking cone equivalence it still computes potential ray indices and then looks for a face with them. It will not work for non-strictly convex cones, where I also check for equivalence. (Although it will not work yet anyway - we cannot compute face lattice in such a case.) Documentation is mostly the same for fans and cones, but I tried to explain and illustrate clearly what the function does and why one should care.
</p>
<p>
I have also tweaked <code>Fan</code> constructor a little bit - now rays of the fan generated by a singe cone will have the same order as this cone. Shuffling bugged me some time before and again now that I was writing doctests for embeddings. My general attitude to such situations is that users enter data in the way they like, so it is good to preserve it as much as possible.
</p>
TicketnovoseltWed, 13 Oct 2010 04:57:52 GMT
https://trac.sagemath.org/ticket/9972#comment:36
https://trac.sagemath.org/ticket/9972#comment:36
<p>
P.S. I also optimized <code>is_equivalent</code> for cones of the same ambient, as was suggested in <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/9972#comment:20"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/9972#comment:20</a>.
</p>
TicketnovoseltWed, 13 Oct 2010 16:01:04 GMT
https://trac.sagemath.org/ticket/9972#comment:37
https://trac.sagemath.org/ticket/9972#comment:37
<p>
Fixes for non-strictly convex cones: <code>cone</code> --> <code>self</code> for convexity check in <code>Cone.embed</code> and extra convexity check in <code>Cone.is_equivalent</code> for the case of common ambient (does not require extra computations for cones of fans, they are always directly set to be strictly convex).
</p>
TicketvbraunThu, 14 Oct 2010 19:13:48 GMT
https://trac.sagemath.org/ticket/9972#comment:38
https://trac.sagemath.org/ticket/9972#comment:38
<p>
I couldn't figure out whether your patch goes before or after mine, so I folded it into one and fixed everything.
</p>
<p>
I changed
</p>
<pre class="wiki"> # Optimization for fans generated by a single cone
if len(cones) == 1:
cone = cones[0]
return RationalPolyhedralFan((tuple(range(cone.nrays())), ),
cone.rays(), lattice,
is_complete=lattice.dimension() > 0)
</pre><p>
to <code>is_complete=(lattice.dimension()==0)</code>.
</p>
<p>
Now that we agree on this ;-) can you go ahead and remove the <code>Enhanced*</code> versions of fans and cones? Then we can go on with fan morphisms...
</p>
TicketnovoseltThu, 14 Oct 2010 23:44:15 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_add_cone_embedding.patch</em>
</li>
</ul>
TicketnovoseltThu, 14 Oct 2010 23:53:17 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_improve_element_constructors.3.patch</em>
</li>
</ul>
TicketnovoseltFri, 15 Oct 2010 00:15:07 GMT
https://trac.sagemath.org/ticket/9972#comment:39
https://trac.sagemath.org/ticket/9972#comment:39
<p>
Oops, sorry - I should have written that it was supposed to be the first. I unfolded the patches back so that it is clear who is writing/reviewing what and we don't need to seek the third person for the final review. I have updated my patch to fix the mistake that you caught. In your part I have removed <code>is_equivalent</code> from <code>Cone_of_fan</code> since this optimization is now performed by general cones. I have also removed extra parenthesis from <code>cone = fan().embed(x)</code>.
</p>
<p>
I also have one more issue with your patch which got lost above, regarding new containment check: cones are equal if they have the same rays in the same order and equivalent if they define the same set of points. If the same cone happened to sit in different fans (or cones, for that matter) and so has several different objects representing it, it does not change anything. We can check if cones belong to the same ambient structure for code optimization, but the output should be the same. Also, to me it feels perfectly natural to ask e.g. whether a cone of some fan belongs to a subdivision of this fan. So I think that <code>cone in fan</code> should return <code>True</code> if <code>cone</code> is equivalent to any of the cones of <code>fan</code>. What is the ambient structure of <code>cone</code> and what is its ray order does not matter. If you really disagree with this, then I think that <code>cone in fan</code> should return <code>True</code> ONLY if <code>cone.ambient() is fan</code> is <code>True</code>. But I definitely prefer the first variant. Then one can write
</p>
<pre class="wiki">sage: if cone in fan:
sage: cone = fan.embed(cone)
sage: Do something, say with cone.adjacent()
sage: else:
sage: Deal with it somehow.
</pre>
TicketnovoseltFri, 15 Oct 2010 00:58:09 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_remove_enhanced_cones_and_fans.patch</em>
</li>
</ul>
TicketnovoseltFri, 15 Oct 2010 01:00:18 GMT
https://trac.sagemath.org/ticket/9972#comment:40
https://trac.sagemath.org/ticket/9972#comment:40
<p>
Enhanced cones are removed, all tests pass. Current ticket queue: last three patches then the first one (which is going to be changed).
</p>
TicketvbraunFri, 15 Oct 2010 08:21:59 GMT
https://trac.sagemath.org/ticket/9972#comment:41
https://trac.sagemath.org/ticket/9972#comment:41
<p>
I agree with <code>cone in fan</code> meaning that cone is equivalent to any cone of the fan. I updated my patch to reflect this.
</p>
TicketvbraunFri, 15 Oct 2010 13:17:07 GMT
https://trac.sagemath.org/ticket/9972#comment:42
https://trac.sagemath.org/ticket/9972#comment:42
<p>
Oops I had forgotten to refresh the patch. Correct version follows.
</p>
TicketvbraunFri, 15 Oct 2010 13:17:30 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_improve_element_constructors.patch</em>
</li>
</ul>
<p>
Updated patch
</p>
TicketnovoseltMon, 18 Oct 2010 02:06:58 GMT
https://trac.sagemath.org/ticket/9972#comment:43
https://trac.sagemath.org/ticket/9972#comment:43
<p>
I'll post an updated patch with <code>FanMorphism</code> shortly.
</p>
<p>
Regarding <code>image_cone</code> and <code>preimage_cones</code> - I guess we want them to work for arbitrary cones of domain/codomain fans. In this case it is still clear what to return for <code>image_cone(cone)</code> - the smallest cone of the codomain fan which contains the image. But what about <code>preimage_cones</code>? Should we return all cones that are mapped to it, or only maximal ones? (We can also consider packing them into a fan, but in this case we loose connection with the domain fan, so I don't think that it is a good idea.)
</p>
<p>
I am also not sure what would be an efficient way to determine preimage cones. I am thinking about determining rays that are mapped into the given codomain cone and then scanning trough all domain cones to see which are generated by such rays only. An alternative approach, which is likely to be better for intensive work with such cones, is to compute full dictionary for image cones and then revert it to get preimage ones. Any suggestions?
</p>
TicketnovoseltMon, 18 Oct 2010 04:41:59 GMTdescription, work_issues, summary changed
https://trac.sagemath.org/ticket/9972#comment:44
https://trac.sagemath.org/ticket/9972#comment:44
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9972?action=diff&version=44">diff</a>)
</li>
<li><strong>work_issues</strong>
changed from <em>switch to FanMorphism</em> to <em>implement preimage_cones</em>
</li>
<li><strong>summary</strong>
changed from <em>Add toric lattice morphisms</em> to <em>Add fan morphisms</em>
</li>
</ul>
<p>
I am attaching a patch that does not compute <code>preimage_cones</code> yet, but the rest is claimed to be ready for review/comments. I have removed classes for lattice morphisms themselves since they were not adding anything anymore. All of the old functionality is moved to <code>FanMorphism</code> constructor as you have suggested above. Codomain fan can now be omitted and will be computed, if possible.
</p>
<p>
This feature has exposed a problem I mentioned (I think) when we were adding warnings to the <code>Fan</code> constructor. The image fan is generated by images of cones of the original fan and these images may coincide or become non-maximal. As a result, one of the doctests fails due to the warning that some cones were discarded and users may see such a message as well, which will be quite confusing, I think. What should we do? Add a parameter to <code>Fan(..., warn=True)</code> and set it to <code>False</code> explicitly in the internal code?
</p>
<p>
Current queue:
</p>
<ul><li>trac_9972_add_cone_embedding.patch
</li><li>trac_9972_improve_element_constructors.patch
</li><li>trac_9972_remove_enhanced_cones_and_fans.patch
</li><li>trac_9972_add_fan_morphisms.patch
</li></ul>
TicketnovoseltWed, 20 Oct 2010 03:30:15 GMTstatus, work_issues changed
https://trac.sagemath.org/ticket/9972#comment:45
https://trac.sagemath.org/ticket/9972#comment:45
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
<li><strong>work_issues</strong>
changed from <em>implement preimage_cones</em> to <em>Warning from Fan constructor</em>
</li>
</ul>
<p>
OK, <code>preimage_cones</code> can now be computed, everything is doctested, the last issue is with warnings.
</p>
TicketnovoseltSat, 23 Oct 2010 18:02:22 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_fix_fan_warning.patch</em>
</li>
</ul>
TicketnovoseltSat, 23 Oct 2010 18:07:22 GMTstatus, reviewer, author changed; work_issues deleted
https://trac.sagemath.org/ticket/9972#comment:46
https://trac.sagemath.org/ticket/9972#comment:46
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Volker Braun</em> to <em>Volker Braun, Andrey Novoseltsev</em>
</li>
<li><strong>work_issues</strong>
<em>Warning from Fan constructor</em> deleted
</li>
<li><strong>author</strong>
changed from <em>Andrey Novoseltsev</em> to <em>Andrey Novoseltsev, Volker Braun</em>
</li>
</ul>
<p>
After some more contemplating, I don't really see any other solution to the problem except for adding a parameter to suppress warnings. (Well, the other option is to remove the warning completely, but I have a feeling that it will not be accepted ;-)) So the last patch does exactly that, now all doctests pass smoothly and I propose merging this ticket.
</p>
<p>
For the record: I am giving positive review to the current "trac_9972_improve_element_constructors.patch" written by Volker Braun.
</p>
TicketnovoseltSat, 23 Oct 2010 21:22:41 GMTdescription changed
https://trac.sagemath.org/ticket/9972#comment:47
https://trac.sagemath.org/ticket/9972#comment:47
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9972?action=diff&version=47">diff</a>)
</li>
</ul>
TicketvbraunMon, 08 Nov 2010 04:25:20 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:48
https://trac.sagemath.org/ticket/9972#comment:48
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Looks good overall, but I think there is a bug in <code>image_cone()</code>:
</p>
<pre class="wiki">sage: N1 = ToricLattice(1)
sage: N2 = ToricLattice(2)
sage: Hom21 = Hom(N2,N1)
sage: pr = Hom21([N1.0,0])
sage: P1xP1=toric_varieties.P1xP1().fan()
sage: f = FanMorphism(pr, P1xP1)
sage: f.codomain_fan().generating_cones()
(1-d cone of Rational polyhedral fan in 1-d lattice N, 1-d cone of Rational polyhedral fan in 1-d lattice N)
sage: [ f.image_cone(c) for c in P1xP1.generating_cones() ]
[0-d cone of Rational polyhedral fan in 1-d lattice N, 0-d cone of Rational polyhedral fan in 1-d lattice N, 0-d cone of Rational polyhedral fan in 1-d lattice N, 0-d cone of Rational polyhedral fan in 1-d lattice N]
</pre>
TicketnovoseltMon, 08 Nov 2010 15:44:47 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:49
https://trac.sagemath.org/ticket/9972#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Thanks for catching it!
</p>
<p>
Fortunately, I still believe that <code>image_cone</code> is correct, but I have tracked down two "degenerate case" bugs: <a class="closed ticket" href="https://trac.sagemath.org/ticket/10237" title="defect: Polyhedra of trivial cones are wrong (closed: fixed)">#10237</a> (I have fixed it and it is sufficient for this ticket to work correctly) and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10238" title="defect: Containment checks are wrong for empty polyhedra (closed: fixed)">#10238</a> (which I have left for you ;-)).
</p>
TicketnovoseltMon, 08 Nov 2010 15:52:06 GMTdescription changed
https://trac.sagemath.org/ticket/9972#comment:50
https://trac.sagemath.org/ticket/9972#comment:50
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9972?action=diff&version=50">diff</a>)
</li>
</ul>
TicketvbraunMon, 08 Nov 2010 19:01:39 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:51
https://trac.sagemath.org/ticket/9972#comment:51
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Ok now <code>image_cone</code> works but I can't compute the preimage:
</p>
<pre class="wiki">sage: c = f.image_cone( Cone([(1,0),(0,1)]) )
sage: c
1-d cone of Rational polyhedral fan in 1-d lattice N
sage: f.preimage_cones(c)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
/home/vbraun/opt/sage-4.6/devel/sage-main/<ipython console> in <module>()
/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/geometry/fan_morphism.pyc in preimage_cones(self, cone)
858 for dcone in dcones:
859 if (possible_rays.issuperset(dcone.ambient_ray_indices())
--> 860 and self.image_cone(dcone).is_face_of(cone)):
861 preimage_cones.append(dcone)
862 self._preimage_cones[cone] = preimage_cones
/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/geometry/fan_morphism.pyc in image_cone(self, cone)
809 RISGIS = self._RISGIS()
810 CSGIS = set(reduce(operator.and_,
--> 811 (RISGIS[i] for i in cone.ambient_ray_indices())))
812 image_cone = codomain_fan.generating_cone(CSGIS.pop())
813 for i in CSGIS:
TypeError: reduce() of empty sequence with no initial value
</pre>
TicketnovoseltMon, 08 Nov 2010 19:41:51 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:52
https://trac.sagemath.org/ticket/9972#comment:52
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Indeed, it does not work and moreover - the error was in <code>image_cone</code>.
</p>
<p>
I have updated the main patch by adding a special branch for the image of the trivial cone and inserted your example into <code>preimage_cones</code> docstring.
</p>
TicketvbraunTue, 09 Nov 2010 16:26:46 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:53
https://trac.sagemath.org/ticket/9972#comment:53
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
Why is <code>preimage_cones</code> not the preimage of <code>image_cones</code>? This confuses me:
</p>
<pre class="wiki">sage: cone = f.image_cone( Cone([(1,0),(0,1)]) )
sage: [ cone is f.image_cone(c) for c in f.preimage_cones(cone) ]
[True, True, True, False, False, False]
</pre><p>
I understand that it is working as documented, but it seems like it would be more useful to actually return the preimage cones. In other words, take preimage cones in the sense of fan morphisms and not in the sense of convex sets.
</p>
<p>
If one is interested in all cones mapping geometrically into a given cone one could always construct the <code>Fan</code> of the (fan morphism) preimage cones.
</p>
TicketnovoseltTue, 09 Nov 2010 22:16:57 GMT
https://trac.sagemath.org/ticket/9972#comment:54
https://trac.sagemath.org/ticket/9972#comment:54
<p>
I am actually not quite sure I completely understand your comments, which makes me think that method names and definitions require some adjustments. How about the following:
</p>
<ul><li><code>cones_mapped_into(cone)</code> returning all cones of the domain fan mapped into the given <code>cone</code> of the codomain fan (this is what <code>preimage_cones</code> does now). They do form a fan, but I am not sure that it is a good idea to construct it, since we will loose connection to the domain fan then.
</li><li><code>cones_mapped_onto(cone)</code> returning all cones of the domain fan whose <code>image_cone</code> is equal to the given <code>cone</code> of the codomain fan. Is this what you are talking about?
</li></ul><p>
In both cases we may add extra parameter to allow returning only maximal cones with these properties. Or we can add a function to <code>fan</code> module like <code>maximal_cones(cones)</code> that will select the maximal ones. In the case of cones of the same fan the selection process is not terribly expensive.
</p>
<p>
As for <code>preimage_cones</code>, I would say that <code>preimage_cone</code> would make perfect sense if it was just the set-theoretic preimage of a given cone, which in general is not strictly-convex and is likely to be not a part of the domain fan. I actually compute these cones internally for generating cones of the codomain fan, but I am not sure they have much value to the end user.
</p>
<p>
Thoughts?
</p>
TicketvbraunTue, 09 Nov 2010 22:54:47 GMT
https://trac.sagemath.org/ticket/9972#comment:55
https://trac.sagemath.org/ticket/9972#comment:55
<p>
I think the geometric image/preimage of a cone is not particularly useful for toric morphisms. A fan morphism maps cones to cones, but does *not* define linear maps of the individual cones. One should think of it as a map from the poset of domain cones to the poset of codomain cones. Or, in geometric terms, maps of the poset of torus orbits to the poset of torus orbits.
</p>
<p>
The usual blow-up example
</p>
<pre class="wiki">sage: c1 = Cone([(1,0),(1,1)])
sage: c2 = Cone([(1,1),(0,1)])
sage: domain_fan = Fan([c1, c2])
sage: codomain_fan = Fan([Cone([(1,0),(0,1)])])
sage: f = FanMorphism(identity_matrix(ZZ,2),domain_fan,codomain_fan)
sage: ray = Cone([(1,1)])
sage: f.image_cone(ray)
2-d cone of Rational polyhedral fan in 2-d lattice N
</pre><p>
means, geometrically, that the orbit closure <code>P^1</code> corresponding to the cone <code>ray</code> (given by the relative star of <code>ray</code>) is mapped to the point corresponding to the <code>f.image_cone(ray)</code>. Conversely, the preimage cones of <code>f.image_cone(ray)</code> are <code>c1</code>, <code>c2</code>, and <code>ray</code>. Geometrically, this means that the preimage of the point <code>f.image_cone(ray)</code> consists of the torus orbits (north pole), (south pole), <code>C^*</code> making up the fiber <code>P^1</code>.
</p>
TicketnovoseltTue, 09 Nov 2010 23:31:01 GMT
https://trac.sagemath.org/ticket/9972#comment:56
https://trac.sagemath.org/ticket/9972#comment:56
<p>
Why a fan morphism does not define linear maps on individual cones? A fan morphism is a linear map between lattices, so it can be restricted to any (compatible) pair of domain/codomain cones and still induce a morphism between corresponding affine varieties. Just specifying mapping of fans as finite sets of cones is not sufficient, e.g. identity morphism and multiplication by 2 are different but obviously would induce the same cone correspondence. So since the actual linear map does matter, it makes sense to use preimages under this linear map. I agree that full (potentially non-strictly convex) preimages are probably of little use, we need to restrict to the domain fan, but I don't see why something called "preimage" should ever exclude the origin. In the sense of orbit closures it does correspond to the whole variety, but in the sense of affine patches it corresponds to the torus itself which is a part of any toric variety of the given dimension and always gets mapped to itself.
</p>
<p>
So I still propose the switch to <code>cones_mapping_into</code> for the current version and adding <code>cones_mapping_onto</code> for the version that you want to have. No <code>preimage_cones</code> at all since it is a confusing name in this context. On the level of toric varieties the corresponding methods may refer to orbits to make things clear, but here they are not present yet.
</p>
TicketvbraunWed, 10 Nov 2010 04:37:16 GMT
https://trac.sagemath.org/ticket/9972#comment:57
https://trac.sagemath.org/ticket/9972#comment:57
<p>
There is clearly important information in how the lattice spanned by a cone is mapped to a sublattice of the lattice spanned by the image cone. But that is just the restriction of the underlying lattice morphism, and not associated to just any cone of the domain. You never need the geometric image/preimage of a cone for toric geometry.
</p>
<p>
I would find it confusing if the <code>FanMorphism</code> contained any other "map of cones" than fan morphisms. Since there is no ambiguity in "image cone", there is no question about "preimage cones" in that category. If you want <code>cones_mapping_into</code> and <code>cones_mapping_onto</code> for implementation purposes that is fine, but <code>image_cone</code> and <code>preimage_cones</code> are what they are and should be called like that.
</p>
TicketnovoseltWed, 10 Nov 2010 05:49:42 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:58
https://trac.sagemath.org/ticket/9972#comment:58
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_work</em>
</li>
</ul>
<p>
I kind of complained before that I don't quite understand the term "fan morphism" ;-)
</p>
<p>
From the beginning of the ticket:
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9972#comment:6" title="Comment 6">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9972#comment:5" title="Comment 5">novoselt</a>:
</p>
<blockquote class="citation">
<p>
"it is a morphism between toric lattices with distinguished fans in the domain and codomain which are compatible with this morphism."
</p>
</blockquote>
<p>
Yes, that is the usual definition. No restriction on the support of the underlying lattice map.
</p>
</blockquote>
<p>
So, as I understand we have a category with fans being objects. A fan is a collection of cones, each cone is a set of points. All cones of the fan sit in the same lattice, so there is also "the lattice of the fan."
</p>
<p>
Morphisms in this category are morphisms between lattices associated to fans, which map cones into cones as sets of points. So in particular they do define linear maps between individual cones as well, and I got lost when you said that they don't.
</p>
<p>
But I guess I agree that when we talk about images/preimages of cones, we should refer to the induced map between cones as a map between two finite sets, since we do not have a nice correspondence between cones as sets of points. Let me think a little more about it and post an updated patch.
</p>
TicketnovoseltWed, 10 Nov 2010 15:44:44 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:59
https://trac.sagemath.org/ticket/9972#comment:59
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I am convinced, the updated patch does what you have suggested and now <code>preimage_tuple</code> returns a tuple, as it was intended and documented. Thanks for the input!
</p>
TicketnovoseltWed, 10 Nov 2010 15:46:09 GMT
https://trac.sagemath.org/ticket/9972#comment:60
https://trac.sagemath.org/ticket/9972#comment:60
<p>
Of course, I meant that <code>preimage_cones</code> returns a tuple...
</p>
TicketvbraunWed, 10 Nov 2010 22:17:59 GMT
https://trac.sagemath.org/ticket/9972#comment:61
https://trac.sagemath.org/ticket/9972#comment:61
<p>
In the aforementioned blow-up example, I get now
</p>
<pre class="wiki">sage: f.preimage_cones(f.image_cone(ray))
(2-d cone of Rational polyhedral fan in 2-d lattice N, 2-d cone of Rational polyhedral fan in 2-d lattice N)
sage: filter(lambda c:f.image_cone(c).is_equivalent(f.image_cone(ray)), flatten(domain_fan.cones()))
[1-d cone of Rational polyhedral fan in 2-d lattice N, 2-d cone of Rational polyhedral fan in 2-d lattice N, 2-d cone of Rational polyhedral fan in 2-d lattice N]
sage: ray in f.preimage_cones(f.image_cone(ray)) # should be True
False
</pre><p>
The <code>preimage_cones</code> misses the 1-d cone that maps to (in the fan morphism sense) the single 2-d cone of the codomain fan.
</p>
TicketnovoseltWed, 10 Nov 2010 22:52:09 GMTattachment set
https://trac.sagemath.org/ticket/9972
https://trac.sagemath.org/ticket/9972
<ul>
<li><strong>attachment</strong>
set to <em>trac_9972_add_fan_morphisms.patch</em>
</li>
</ul>
<p>
preimage_cones implemented
</p>
TicketnovoseltWed, 10 Nov 2010 22:56:37 GMT
https://trac.sagemath.org/ticket/9972#comment:62
https://trac.sagemath.org/ticket/9972#comment:62
<p>
Grrr... That was due to my optimization attempt without proper thinking. The new version uses the same cycle as the very first one (which was finding everything and even more), but requires equality of image cones. Should work now, the blow up example in the documentation is extended to include the preimage cones of the quadrant...
</p>
TicketvbraunSun, 14 Nov 2010 19:57:00 GMTstatus changed
https://trac.sagemath.org/ticket/9972#comment:63
https://trac.sagemath.org/ticket/9972#comment:63
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Works great now!
</p>
TicketjdemeyerSat, 20 Nov 2010 10:05:56 GMTmilestone changed
https://trac.sagemath.org/ticket/9972#comment:64
https://trac.sagemath.org/ticket/9972#comment:64
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.6.1</em> to <em>sage-4.6.2</em>
</li>
</ul>
TicketvbraunSat, 11 Dec 2010 12:05:00 GMT
https://trac.sagemath.org/ticket/9972#comment:65
https://trac.sagemath.org/ticket/9972#comment:65
<p>
For the tracbot:
</p>
<p>
Apply trac_9972_add_cone_embedding.patch, trac_9972_improve_element_constructors.patch, trac_9972_remove_enhanced_cones_and_fans.patch, trac_9972_add_fan_morphisms.patch, trac_9972_fix_fan_warning.patch
</p>
TicketjdemeyerWed, 12 Jan 2011 06:33:42 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/9972#comment:66
https://trac.sagemath.org/ticket/9972#comment:66
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.6.2.alpha0</em>
</li>
</ul>
Ticket