Opened 9 years ago

Closed 9 years ago

#13191 closed enhancement (fixed)

Construct a 2-d fan from rays only

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

Description (last modified by 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

Attachments (2)

trac_13191_auto_fan_2d.patch (7.2 KB) - added by vbraun 9 years ago.
Updated patch
trac_13191_reviewer.patch (1.9 KB) - added by novoselt 9 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by vbraun

  • Cc novoselt added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by vbraun

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

comment:3 Changed 9 years ago by novoselt

  • Component changed from algebraic geometry to geometry
  • Keywords toric added
  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to needs_work

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

comment:4 Changed 9 years ago by 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.

comment:5 Changed 9 years ago by novoselt

  • Dependencies set to #12544

comment:6 Changed 9 years ago by novoselt

  • Work issues set to completeness bug
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

comment:7 Changed 9 years ago by 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.

Changed 9 years ago by vbraun

Updated patch

comment:8 Changed 9 years ago by vbraun

  • Status changed from needs_work to needs_review
  • Work issues completeness bug deleted

Changed 9 years ago by novoselt

comment:9 Changed 9 years ago by 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!

comment:10 Changed 9 years ago by vbraun

  • Status changed from needs_review to positive_review

Ok now I get it ;-)

Your patch look good to me!

comment:11 Changed 9 years ago by jdemeyer

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