Sage: Ticket #13191: Construct a 2-d fan from rays only
https://trac.sagemath.org/ticket/13191
<p>
It would be convenient to construct a 2-d fan just from rays (by creating the maximal number of 2-d cones). This is what this patch does.
</p>
<pre class="wiki">sage: fan = Fan2d([(1,1), (-1,-1), (1,-1), (-1,1)])
sage: [ cone.ambient_ray_indices() for cone in fan ]
[(2, 1), (1, 3), (3, 0), (0, 2)]
sage: fan.is_complete()
True
</pre>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/13191
Trac 1.1.6vbraunSun, 01 Jul 2012 20:51:01 GMTstatus, description changed; cc set
https://trac.sagemath.org/ticket/13191#comment:1
https://trac.sagemath.org/ticket/13191#comment:1
<ul>
<li><strong>cc</strong>
<em>novoselt</em> added
</li>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/13191?action=diff&version=1">diff</a>)
</li>
</ul>
TicketvbraunMon, 02 Jul 2012 13:22:54 GMT
https://trac.sagemath.org/ticket/13191#comment:2
https://trac.sagemath.org/ticket/13191#comment:2
<p>
The updated patch removes duplicated rays (without changing the order) to match the usual <code>Fan()</code> behavior.
</p>
TicketnovoseltSat, 07 Jul 2012 14:12:20 GMTstatus, component changed; keywords, reviewer set
https://trac.sagemath.org/ticket/13191#comment:3
https://trac.sagemath.org/ticket/13191#comment:3
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>keywords</strong>
<em>toric</em> added
</li>
<li><strong>component</strong>
changed from <em>algebraic geometry</em> to <em>geometry</em>
</li>
<li><strong>reviewer</strong>
set to <em>Andrey Novoseltsev</em>
</li>
</ul>
<p>
How about expanding one-liner
</p>
<pre class="wiki">Construct a 2-dimensional fan
</pre><p>
into
</p>
<pre class="wiki">Construct the maximal 2-d fan with given ``rays``.
</pre><p>
?
</p>
<p>
It would be nice also to have a more descriptive name, maybe <code>FanFromRays</code>? And perhaps one example can go to the module-level documentation to increase visibility of the constructor. (Or it can be mentioned in the documentation of <code>Fan</code>.)
</p>
<p>
I also believe that one more case needs special treatment: two opposite rays, in which case there are two 1-d generating cones. (So far care was taken of no rays at all and a single ray.)
</p>
TicketvbraunSun, 08 Jul 2012 11:01:23 GMT
https://trac.sagemath.org/ticket/13191#comment:4
https://trac.sagemath.org/ticket/13191#comment:4
<p>
I've added it more prominently to the documentation and fixed the opposite ray case.
</p>
<p>
I think <code>FanFromRays</code> is confusing since it only works in 2d. Right now tab-completion will tell you that there is a <code>Fan2d</code>, so if that is what you want then its clear that it is the right method. For experts its clear that this means you don't need to specify cones manually, but if there is any question there is always the online help.
</p>
TicketnovoseltSun, 08 Jul 2012 14:42:07 GMTdependencies set
https://trac.sagemath.org/ticket/13191#comment:5
https://trac.sagemath.org/ticket/13191#comment:5
<ul>
<li><strong>dependencies</strong>
set to <em>#12544</em>
</li>
</ul>
TicketnovoseltSun, 08 Jul 2012 15:53:22 GMTwork_issues set
https://trac.sagemath.org/ticket/13191#comment:6
https://trac.sagemath.org/ticket/13191#comment:6
<ul>
<li><strong>work_issues</strong>
set to <em>completeness bug</em>
</li>
</ul>
<pre class="wiki">sage: Fan2d([(0,1)])
Rational polyhedral fan in 2-d lattice N
sage: _.is_complete()
True
</pre><p>
Selecting distinct rays is a bit convoluted, but especially strange is repeating this selection on sorted rays which are already distinct just to trigger the complete removal of a single ray. How about something like this:
</p>
<pre class="wiki">...
n = len(rays)
if n == 1 or n == 2 and rays[0] == -rays[1]:
cones = [(i, ) for i in range(n)]
return RationalPolyhedralFan(cones, rays, lattice, False)
import math
sorted_rays = sorted(...)
cones = []
...
# there are definitely some 2-d cones now
</pre>
TicketvbraunSun, 08 Jul 2012 16:38:18 GMT
https://trac.sagemath.org/ticket/13191#comment:7
https://trac.sagemath.org/ticket/13191#comment:7
<p>
Just as in the <code>Fan()</code> constructor, it is permissible to pass an arbitrary number of duplicate rays. For example:
</p>
<pre class="wiki">sage: Fan2d([(0,1), (1,0), (0,1), (1,0)])
Rational polyhedral fan in 2-d lattice N
</pre><p>
I fixed the completeness bug.
</p>
TicketvbraunSun, 08 Jul 2012 16:38:36 GMTattachment set
https://trac.sagemath.org/ticket/13191
https://trac.sagemath.org/ticket/13191
<ul>
<li><strong>attachment</strong>
set to <em>trac_13191_auto_fan_2d.patch</em>
</li>
</ul>
<p>
Updated patch
</p>
TicketvbraunSun, 08 Jul 2012 16:38:51 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/13191#comment:8
https://trac.sagemath.org/ticket/13191#comment:8
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>completeness bug</em> deleted
</li>
</ul>
TicketnovoseltSun, 08 Jul 2012 17:26:39 GMTattachment set
https://trac.sagemath.org/ticket/13191
https://trac.sagemath.org/ticket/13191
<ul>
<li><strong>attachment</strong>
set to <em>trac_13191_reviewer.patch</em>
</li>
</ul>
TicketnovoseltSun, 08 Jul 2012 17:28:22 GMT
https://trac.sagemath.org/ticket/13191#comment:9
https://trac.sagemath.org/ticket/13191#comment:9
<p>
I was complaining not about removing duplicates - that is fine - but about the way how it is done. Nonetheless, it is still clear what is going on. But the line
</p>
<pre class="wiki">sorted_rays = [ r for i,r in enumerate(sorted_rays) if r[1] != sorted_rays[i-1][1] ]
</pre><p>
is very confusing: sorted rays are already unique and the equality can be triggered only when the first element is equal to the previous, i.e. the last one, which is the same as first when the length is one. In other words the effect of this line is the same as
</p>
<pre class="wiki">if len(sorted_rays) == 1: sorted_rays = []
</pre><p>
and it does require some thinking to figure out that this is what happening and then realize how the rest of the code depends on it.
</p>
<p>
So I still propose to unwind the code a bit, if you agree with the changes in the reviewer patch, please set it to positive review!
</p>
TicketvbraunSun, 08 Jul 2012 18:35:18 GMTstatus changed
https://trac.sagemath.org/ticket/13191#comment:10
https://trac.sagemath.org/ticket/13191#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Ok now I get it ;-)
</p>
<p>
Your patch look good to me!
</p>
TicketjdemeyerFri, 13 Jul 2012 11:44:15 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/13191#comment:11
https://trac.sagemath.org/ticket/13191#comment:11
<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-5.2.beta1</em>
</li>
</ul>
Ticket