Construct a 2-d fan from rays only

[vbraun]

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.

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

[vbraun]

The updated patch removes duplicated rays (without changing the order) to match the usual Fan() behavior.

[novoselt]

How about expanding one-liner

Construct a 2-dimensional fan

into

Construct the maximal 2-d fan with given ``rays``.

?

It would be nice also to have a more descriptive name, maybe FanFromRays? 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 Fan.)

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

[vbraun]

I've added it more prominently to the documentation and fixed the opposite ray case.

I think FanFromRays is confusing since it only works in 2d. Right now tab-completion will tell you that there is a Fan2d, 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.

[novoselt]

sage: Fan2d([(0,1)])
Rational polyhedral fan in 2-d lattice N
sage: _.is_complete()
True

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:

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

[vbraun]

Just as in the Fan() constructor, it is permissible to pass an arbitrary number of duplicate rays. For example:

sage: Fan2d([(0,1), (1,0), (0,1), (1,0)])
Rational polyhedral fan in 2-d lattice N

I fixed the completeness bug.

[vbraun]

Updated patch

[novoselt]

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

sorted_rays = [ r for i,r in enumerate(sorted_rays) if r[1] != sorted_rays[i-1][1] ] 

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

if len(sorted_rays) == 1: sorted_rays = []

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.

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!

[vbraun]

Ok now I get it ;-)

Your patch look good to me!