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 )
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)
Change History (13)
comment:1 Changed 9 years ago by
- Cc novoselt added
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
- 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
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
- Dependencies set to #12544
comment:6 Changed 9 years ago by
- 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
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.
comment:8 Changed 9 years ago by
- Status changed from needs_work to needs_review
- Work issues completeness bug deleted
Changed 9 years ago by
comment:9 Changed 9 years ago by
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
- 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
- Merged in set to sage-5.2.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
The updated patch removes duplicated rays (without changing the order) to match the usual
Fan()
behavior.