Opened 6 years ago
Closed 5 years ago
#22552 closed defect (fixed)
2 bugs creating a simple 2point Polytope
Reported by:  William Stein  Owned by:  

Priority:  minor  Milestone:  sage8.1 
Component:  geometry  Keywords:  base ring, polyhedron, days88, IMA coding sprints 
Cc:  JeanPhilippe Labbé, Moritz Firsching, Matthias Köppe, Vincent Delecroix  Merged in:  
Authors:  JeanPhilippe Labbé  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  150c7df (Commits, GitHub, GitLab)  Commit:  150c7df7962e6ddb06583511e907c1cf6606b863 
Dependencies:  Stopgaps: 
Description
Sara Billey (of Univ of Washington) reported these.
~/Sara$ sagedevelop ┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 7.6.beta5, Release Date: 20170226 │ │ Type "notebook()" for the browserbased notebook interface. │ │ Type "help()" for help. │ └────────────────────────────────────────────────────────────────────┘ ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ ┃ Warning: this is a prerelease version, and it may be unstable. ┃ ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ sage: P=Polyhedron(vertices =[(8.3319544851638732, 7.0567045956967727), (6.4876921900819049, 4.8435898415984129)]) sage: P The empty polyhedron in (Real Field with 57 bits of precision)^2 sage: # WRONG! It should not be empty. Indeed, look: sage: P=Polyhedron(vertices =[(8.3319544851638732, 7.0567045956967727), (6.4876921900819049, 4.84358984159841)]) sage: P A 1dimensional polyhedron in RDF^2 defined as the convex hull of 2 vertices sage: # Also, here's a hub traceback for no reason (as a second but maybe related bug): sage: P=Polyhedron(vertices =[(8.3319544851638732, 7.0567045956967727), (6.4876921900819049, 4.8435898415984129)], base_ring=RealField(40)) sage: P.plot()  KeyError Traceback (most recent call last) <ipythoninput8d297b4e23e6b> in <module>() > 1 P.plot() /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/base.pyc in plot(self, point, line, polygon, wireframe, fill, projection_direction, **kwds) 694 return polyhedron.projection() 695 > 696 projection = project(self) 697 try: 698 plot_method = projection.plot /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/base.pyc in project(polyhedron) 692 return polyhedron.schlegel_projection() 693 else: > 694 return polyhedron.projection() 695 696 projection = project(self) /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/base.pyc in projection(self) 3759 """ 3760 from .plot import Projection > 3761 self.projection = Projection(self) 3762 return self.projection 3763 /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/plot.py in __init__(self, polyhedron, proj) 492 493 if polyhedron.ambient_dim() == 2: > 494 self._init_from_2d(polyhedron) 495 elif polyhedron.ambient_dim() == 3: 496 self._init_from_3d(polyhedron) /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/plot.py in _init_from_2d(self, polyhedron) 748 self.dimension = 2 749 self._init_points(polyhedron) > 750 self._init_lines_arrows(polyhedron) 751 self._init_area_2d(polyhedron) 752 /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/plot.py in _init_lines_arrows(self, polyhedron) 812 if not obj[i].is_vertex(): continue 813 for j in range(len(obj)): > 814 if polyhedron.vertex_adjacency_matrix()[i,j] == 0: continue 815 if i < j and obj[j].is_vertex(): 816 l = [obj[i].vector(), obj[j].vector()] /projects/sage/sagedev/src/sage/misc/cachefunc.pyx in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/projects/sage/sagedev/src/build/cythonized/sage/misc/cachefunc.c:13453)() 2399 if self.cache is None: 2400 f = self.f > 2401 self.cache = f(self._instance) 2402 return self.cache 2403 /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/base.pyc in vertex_adjacency_matrix(self) 1933 (0, 0, 1, 1, 0) A vertex at (3, 0) 1934 """ > 1935 return self._vertex_adjacency_matrix() 1936 1937 adjacency_matrix = vertex_adjacency_matrix /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/base.pyc in _vertex_adjacency_matrix(self) 318 M[j, i] = 1 319 > 320 face_lattice = self.face_lattice() 321 for face in face_lattice: 322 Vrep = face.ambient_Vrepresentation() /projects/sage/sagedev/src/sage/misc/cachefunc.pyx in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/projects/sage/sagedev/src/build/cythonized/sage/misc/cachefunc.c:13453)() 2399 if self.cache is None: 2400 f = self.f > 2401 self.cache = f(self._instance) 2402 return self.cache 2403 /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/base.pyc in face_lattice(self) 3408 return Hasse_diagram_from_incidences\ 3409 (atoms_incidences, coatoms_incidences, > 3410 face_constructor=face_constructor, required_atoms=atoms_vertices) 3411 3412 def faces(self, face_dimension): /projects/sage/sagedev/local/lib/python2.7/sitepackages/sage/geometry/hasse_diagram.pyc in Hasse_diagram_from_incidences(atom_to_coatoms, coatom_to_atoms, face_constructor, required_atoms, key, **kwds) 180 # Make sure that coatoms are in the end in proper order 181 tail = [faces[atoms, frozenset([coatom])] > 182 for coatom, atoms in enumerate(coatom_to_atoms)] 183 tail.append(faces[A, frozenset()]) 184 new_order = [n for n in new_order if n not in tail] + tail KeyError: (frozenset([]), frozenset([0])) sage:
See public worksheet: https://cloud.sagemath.com/projects/53b9d6b6ce2c4007843a257cc01bf65b/files/Sara/Polygon%20Bug.sagews
Change History (36)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
Cc:  JeanPhilippe Labbé Moritz Firsching Matthias Köppe added 

comment:3 Changed 6 years ago by
Branch:  → u/jipilab/22552 

comment:4 Changed 6 years ago by
Authors:  → JeanPhilippe Labbé 

Cc:  Vincent Delecroix added 
Commit:  → a098bc11f9066736c002819b869fd28c1aea310c 
Keywords:  base ring polyhedron added 
Status:  new → needs_review 
comment:5 Changed 6 years ago by
Commit:  a098bc11f9066736c002819b869fd28c1aea310c → 0ff050f294e1c9112fe5ff686df9f82cf267a61a 

Branch pushed to git repo; I updated commit sha1. New commits:
0ff050f  pep8 cleaning

comment:6 followup: 9 Changed 6 years ago by
Do you really want to keep this behavior?
If the input consists of decimal numbers and the `base_ring` is not specified, the base ring is set to be the `RealField` with the precision given by the minimal precision of the input. If it has 53 bits of precision, the constructor converts automatically the base ring to `RDF`::
comment:8 Changed 6 years ago by
An example
sage: float("2.0") ** 2048r == 0.0r True sage: RR(2.) ** 2048r == RR.zero() False
comment:9 followup: 12 Changed 6 years ago by
Replying to vdelecroix:
Do you really want to keep this behavior?
If the input consists of decimal numbers and the `base_ring` is not specified, the base ring is set to be the `RealField` with the precision given by the minimal precision of the input. If it has 53 bits of precision, the constructor converts automatically the base ring to `RDF`::
No, not really...
But at least describing the current behavior may be useful to understand how the constructor works. Is that reasonable?
Once this behavior is changed, I'd just delete the paragraph.
comment:10 Changed 6 years ago by
This ticket only looks at the bugs, now with #18220 merged, the behavior is changed and the "bugs" have now an explanation.
What else should be done here?
comment:11 Changed 6 years ago by
Should the "Do what I mean" be changed completely?
Or should the base_ring passed by the constructor to the parent *always* be RDF
and not RR
?
comment:12 Changed 6 years ago by
Replying to jipilab:
Replying to vdelecroix:
Do you really want to keep this behavior?
If the input consists of decimal numbers and the `base_ring` is not specified, the base ring is set to be the `RealField` with the precision given by the minimal precision of the input. If it has 53 bits of precision, the constructor converts automatically the base ring to `RDF`::No, not really...
But at least describing the current behavior may be useful to understand how the constructor works. Is that reasonable?
When documented, a bug becomes a feature! What about
.. WARNING:: When used with input in the realf field with 53 bits of mantissa precision the input is converted without notice into the machine floating point numbers. This behavior might cause some undesirable effects.
comment:13 Changed 6 years ago by
An example of undesirable feature
sage: a = RR(2.0)**2048 sage: b = RR(3.0)**2048 sage: Polyhedron([[a],[b]]) A 0dimensional polyhedron in RDF^1 defined as the convex hull of 1 vertex
comment:16 Changed 6 years ago by
Further, the warning bugs me a bit...
It does not tell the complete truth about the behaviour: you only need 1 entry to have a 53 bit mentissa to get into floating points not necessary all of them... This is even worse I find! This is somehow the source of the weird behavior described in this ticket. The fact that one value had 53 bit of mentissa made it possible to create the object, and I would say lead to the belief that the polyhedron is well defined.
sage: c = 1.00000000000000 sage: d = 1.000000000000001 sage: Polyhedron([[c],[d]]) A 0dimensional polyhedron in RDF^1 defined as the convex hull of 1 vertex
comment:17 Changed 6 years ago by
I know not everyone is a fan of issuing a warning, but IMHO, I would find it a useful hint to the user if:
 it gave entries with more than 53 bits of mentissa
 it also gave entries with exactly 53 bits of mentissa (not in
ZZ
orQQ
)
To say:
Warning: some entries had more precision than 53 bits and got converted to 53 bits of precision.
This would not be issued if no entries had more precision than 53 bits.
comment:18 Changed 6 years ago by
Status:  needs_review → needs_work 

comment:19 Changed 6 years ago by
Dependencies:  → #22605 

We should probably add a dependancy to #22605 because the latter touches the constructor and floats...
comment:20 Changed 5 years ago by
Dependencies:  #22605 

Milestone:  sage7.6 → sage8.1 
This is indeed fixed in 8.1.beta2 in which #22605 is merged
sage: P=Polyhedron(vertices =[(8.3319544851638732, 7.0567045956967727), (6.4876921900819049, 4.8435898415984129)]) Traceback (most recent call last): ... ValueError: no appropriate backend for computations with Real Field with 57 bits of precision sage: P=Polyhedron(vertices =[(8.3319544851638732, 7.0567045956967727), (6.4876921900819049, 4.84358984159841)]) sage: P A 1dimensional polyhedron in RDF^2 defined as the convex hull of 2 vertices
comment:21 Changed 5 years ago by
Commit:  0ff050f294e1c9112fe5ff686df9f82cf267a61a → 92b9dd0682ca20c611ab659d0bae0b6fe21f54f1 

Branch pushed to git repo; I updated commit sha1. New commits:
92b9dd0  Merge branch 'u/jipilab/22552' of git://trac.sagemath.org/sage into 22552

comment:22 Changed 5 years ago by
Commit:  92b9dd0682ca20c611ab659d0bae0b6fe21f54f1 → c5ff31ec57b9afcc5a799c7ea9a1f3c0a0f0a85c 

Branch pushed to git repo; I updated commit sha1. New commits:
c5ff31e  Fixed failing tests

comment:23 Changed 5 years ago by
Status:  needs_work → needs_review 

Tests seem to pass on 8.1.beta2 but it would be nice to have feedback on the added documentation.
comment:24 Changed 5 years ago by
You should be more explicit about what is meant by precision
sage: (1.1).precision() 53 sage: (1.143234134123123).precision() 54 sage: (1.14323413412312123).precision() 60
It is not the number of digits but closer to min(53, bits(x))
.
comment:25 Changed 5 years ago by
This is not the syntax for test blocks
.. TESTS:
see http://doc.sagemath.org/html/en/developer/coding_basics.html#documentationstrings
comment:26 Changed 5 years ago by
Since there is four examples in the examples, I don't see why the 'TESTS' section is relevant in your commit.
comment:27 Changed 5 years ago by
Keywords:  days88 IMA coding sprints added 

Reviewers:  → Vincent Delecroix 
Status:  needs_review → needs_work 
comment:28 Changed 5 years ago by
Commit:  c5ff31ec57b9afcc5a799c7ea9a1f3c0a0f0a85c → 50cb0c75e937237d4889272dc2da84bce2108b26 

Branch pushed to git repo; I updated commit sha1. New commits:
50cb0c7  Clarified precision and moved tests

comment:29 Changed 5 years ago by
Dear Vincent,
I made some corrections addressing your comments. Have a look.
comment:30 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:31 followup: 32 Changed 5 years ago by
This sentence is definitely too complicated
If the input consists of decimal numbers and the `base_ring` is not specified, the base ring is set to be the `RealField` with the precision given by the minimal bit precision of the input and 53 (the precision of `RDF`). Then, if the obtained minimum was 53 bits of precision, the constructor converts automatically the base ring to `RDF`. Otherwise, it returns an error
I would try to be more informative
Be careful when you construct polyhedra with floating point numbers. The only available backend for such computation is `cdd` which uses machine floating point numbers which have have limited precision (24 bits on 32 bits architecture and 53 bits on 64 bits architecture). Sage behavior ... etc ...
BTW, what is happening on 32 bits?
comment:32 Changed 5 years ago by
BTW, what is happening on 32 bits?
It seems that binary64 format from
https://en.wikipedia.org/wiki/IEEE_754
has 53 bits of precision no matter if it was compiled in 32 or 64 bits processors. I'm no expert here but it seems to be safe to say that a Double
has 53 bits in 32 and 64 bits.
comment:33 Changed 5 years ago by
Commit:  50cb0c75e937237d4889272dc2da84bce2108b26 → ee54e3008b6fa52797892527ed0f0775ee707e9e 

Branch pushed to git repo; I updated commit sha1. New commits:
ee54e30  Rephrased and made it into a warning

comment:34 Changed 5 years ago by
Branch:  u/jipilab/22552 → u/vdelecroix/22552 

Commit:  ee54e3008b6fa52797892527ed0f0775ee707e9e → 150c7df7962e6ddb06583511e907c1cf6606b863 
A (bad) undocumented feature
sage: Polyhedron(vertices =[(8.3319544851638732, 7.0567045956967727), (6.4876921900819049, 4.8435898415984129)], base_ring=RR) A 1dimensional polyhedron in RDF^2 defined as the convex hull of 2 vertices
As agreed with JP, I wrote a commit that makes the above an error.
New commits:
150c7df  22552: disallow base_ring=RealField(n)

comment:35 Changed 5 years ago by
Status:  needs_review → positive_review 

The last changes look good to me. I give my green light on the last changes.
comment:36 Changed 5 years ago by
Branch:  u/vdelecroix/22552 → 150c7df7962e6ddb06583511e907c1cf6606b863 

Resolution:  → fixed 
Status:  positive_review → closed 
If I understood correctly, #18220 tries to fix this and other problems by disallowing inexact fields completely...