Opened 8 years ago
Closed 3 years ago
#17339 closed defect (fixed)
Polyhedron class mistreats empty inputs
Reported by:  Darij Grinberg  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  
Cc:  Volker Braun, Matthias Köppe, Travis Scrimshaw  Merged in:  
Authors:  Darij Grinberg, Jonathan Kliem  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  c5bf041 (Commits, GitHub, GitLab)  Commit:  c5bf04100b1b1bbd32e7c85e0096f69934948811 
Dependencies:  Stopgaps: 
Description (last modified by )
sage: Polyhedron(ambient_dim=0, ieqs=[], eqns=[], base_ring=QQ) The empty polyhedron in QQ^0
This should be a onepoint polyhedron.
sage: Polyhedron(ambient_dim=2, ieqs=[], eqns=[], base_ring=QQ) The empty polyhedron in QQ^2
This should be the whole QQ^{2. }
sage: Polyhedron(ambient_dim=2, vertices=[], rays=[], lines=[], base_ring=QQ) The empty polyhedron in QQ^2
This is correct, but only because the code misunderstands the empty Vrepresentation as no Vrepresentation given at all.
Change History (22)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
There is a difference between vertices=[]
and vertices=None
. In the latter case, we should follow the cdd convention. In the former, we should not; this kind of exception conflicts with regular usage and forces writers of methods to specialcase empty vertex sets. Currently the constructor loses the difference between []
and None
at one of its first steps (the _make_listlist
call); this is what I think needs to be fixed.
comment:3 Changed 8 years ago by
I don't really like to make a distinction between None
and []
, Python generally treats them as similar. Both are not __nonzero__
. Its a bad UI to assign meaning to one over the other.
comment:4 Changed 8 years ago by
What we have right now is way worse. Say I write some code which constructs a ton of polytopes from their Vreps (e.g., I have a polygon and I construct all subpolygons from some of its vertices). A few of them have empty vertices lists. Why should they be understood differently from the rest?
comment:5 Changed 8 years ago by
The design choice is: nothing specified = empty polyhedron.
This is fine with your example, empty vertex list = empty polygon. Unless you mean a noncompact 2d polyhedron.
I agree that there is perhaps too much DWIM but its also very convenient for interactive use. If you just specify a bunch of rays then in 99% of the cases you want a cone, not some rule thumping about missing a vertex.
How about separate Polyhedron.from_Vrep(vertices, rays, lines)
and similarly from_Hrep
with positional arguments and strict rules?
comment:6 followup: 7 Changed 8 years ago by
Very good idea about the from_Vrep
and from_Hrep
methods. Should the usual __init__
then dispatch to them?
comment:7 Changed 8 years ago by
Replying to darij:
Should the usual
__init__
then dispatch to them?
Haven't really thought about it, whatever is convenient for the implementation.
comment:8 Changed 6 years ago by
Cc:  Matthias Köppe added 

comment:9 Changed 6 years ago by
Even if it's not good style to distinguish between None
and []
, I think it is fine to distinguish between a provided keyword argument and a notprovided keyword argument.
So perhaps we should just change the default value of eqns from None
to some unique value such as "not_given"
.
Then users who want to explicitly pass an empty list of eqns could either pass eqns=None
or eqns=[]
.
comment:10 Changed 4 years ago by
Cc:  Travis Scrimshaw added 

Milestone:  sage6.4 → sage8.4 
comment:11 Changed 4 years ago by
I think Python quite often distinguishes between []
and None
:
sage: list([]) [] sage: list(None)  TypeError Traceback (most recent call last) <ipythoninput1f23beda2b661> in <module>() > 1 list(None) TypeError: 'NoneType' object is not iterable sage: for x in []: ....: print("hi") sage: for x in None: ....: print("hi")  TypeError Traceback (most recent call last) <ipythoninput5b89ce2509dd6> in <module>() > 1 for x in None: 2 print("hi") TypeError: 'NoneType' object is not iterable
They are completely different objects in Python. Other languages behave in the same way for their listlike objects and equivalent of NULL
.
I agree that it can be useful to have []
and None
behave the same way, but it is not good practice to say we should not distinguish between the two. None
is also faster, easier, and safer to work with than some other (essentially equally) generic input.
comment:12 Changed 3 years ago by
Authors:  → Darij Grinberg, Jonathan Kliem 

Branch:  public/polytopes/0 → public/17339 
Commit:  f7642d6e2d2502e3df95c76ef2ffa7e9319a99aa → f3c0bfbba740e2c001aa0ad15a47e956c8181c0d 
I took the halfbacked fix and did some changes that seem to make it work.
It seems that fixing this ticket implies a fixing #28654. The verification of the double description for backend field
relied on mistreatment of empty input (or fixing #28654).
New commits:
f3c0bfb  fix empty input for Polyhedron class

comment:13 Changed 3 years ago by
Status:  new → needs_review 

comment:14 Changed 3 years ago by
Commit:  f3c0bfbba740e2c001aa0ad15a47e956c8181c0d → c5bf04100b1b1bbd32e7c85e0096f69934948811 

Branch pushed to git repo; I updated commit sha1. New commits:
c5bf041  added a doctest, some simplification

comment:15 Changed 3 years ago by
Description:  modified (diff) 

Milestone:  sage8.4 → sage9.0 
I propose leaving the following as it is:
sage: Polyhedron(ambient_dim=2, vertices=[], rays=[(2,2)], lines=[], base_ring=QQ) A 1dimensional polyhedron in QQ^2 defined as the convex hull of 1 vertex and 1 ray
comment:16 Changed 3 years ago by
Should we consider that as valid input? Given a single ray, I don't think there is enough information to determine a polyhedron. However, since that is the current behavior, I cannot strictly object to it. I am only with accepting the ticket asis since it does improve things, but perhaps we can take a quick second look at the current corner case behaviors?
comment:17 Changed 3 years ago by
I think if vertices is None
then it makes sense to add a vertex.
However, if vertices is []
and there are Rays or Lines, maybe it makes sense to throw an error. The user specifically stated that there are no vertices, so the input doesn't make any sense.
comment:18 Changed 3 years ago by
But you cannot infer what the vertices should be from a single ray (or even line). This should be true more generally for any nonfulldimensional polyhedron that cannot specify at least one point inside it. However, as I write that, it seems like a (computationally) hard problem to me to verify this information. Although maybe this is already implicit in how the polyhedron is constructed?
comment:19 Changed 3 years ago by
I guess it is just convention, to add a vertex, when rays and lines are given, but no vertex. I don't really care about that convention one way or the other. I just left it the way it is.
I don't understand what part is computationally hard.
comment:20 Changed 3 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
I was thinking given enough rays, but now I realize that no matter how much affine data you have, you need to specify a vertex. However, what about a mixture of lines and rays. Although I guess it is documented that the origin is added if no vertices are specified, so it is enshrined as the behavior. Thus this point is moot.
comment:21 Changed 3 years ago by
Authors:  Darij Grinberg, Jonathan Kliem → Darij Grinberg, Jonathan Kliem 

fixed zero width space in the author name
comment:22 Changed 3 years ago by
Branch:  public/17339 → c5bf04100b1b1bbd32e7c85e0096f69934948811 

Resolution:  → fixed 
Status:  positive_review → closed 
For the case with rays but no vertex we follow cdd in implicitly adding the origin as vertex. This is also spelled out in the docs.