Opened 4 years ago
Closed 4 years ago
#26340 closed enhancement (fixed)
polytopes.snub_cube should allow exact coordinates
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  geometry  Keywords:  snub_cube, polytopes 
Cc:  Nico Van Cleemput, JeanPhilippe Labbé, Laith Rastanawi  Merged in:  
Authors:  Laith Rastanawi, Matthias Koeppe  Reviewers:  Vincent Delecroix, JeanPhilippe Labbé, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  13d8c44 (Commits, GitHub, GitLab)  Commit:  13d8c44c9f73ac3f7033fc819781a6e349f1637a 
Dependencies:  Stopgaps: 
Description (last modified by )
polytopes.snub_cube
should allow to have vertices with exact coordinates.
In preparation for #25097, the method has the new optional parameters verbose
, base_ring
, and exact
.
Change History (21)
comment:1 Changed 4 years ago by
Cc:  Laith Rastanawi added 

comment:2 Changed 4 years ago by
Branch:  → public/26340 

Commit:  → 57a9d108994a33efedfaf4b6d6732358db882800 
Keywords:  snub_cube polytopes added 
Milestone:  sage8.4 → sage8.8 
Status:  new → needs_review 
comment:3 Changed 4 years ago by
Authors:  → Laith Rastanawi 

comment:4 Changed 4 years ago by
Reviewers:  → vincent Delecroix 

Status:  needs_review → needs_work 
sage t long src/doc/en/thematic_tutorials/geometry/polyhedra_tutorial.rst ********************************************************************** File "src/doc/en/thematic_tutorials/geometry/polyhedra_tutorial.rst", line 770, in doc.en.thematic_tutorials.geometry.polyhedra_tutorial Failed example: E = polytopes.snub_cube(); E Expected: A 3dimensional polyhedron in RDF^3 defined as the convex hull of 24 vertices Got: A 3dimensional polyhedron in (Number Field in z with defining polynomial x^3 + x^2 + x  1)^3 defined as the convex hull of 24 vertices ********************************************************************** 1 item had failures: 1 of 109 in doc.en.thematic_tutorials.geometry.polyhedra_tutorial [108 tests, 1 failure, 57.00 s]  sage t long src/doc/en/thematic_tutorials/geometry/polyhedra_tutorial.rst # 1 doctest failed 
comment:5 Changed 4 years ago by
Commit:  57a9d108994a33efedfaf4b6d6732358db882800 → 8703f7f85aa7b7f0161a8c90fa92dd0d2e47dc70 

Branch pushed to git repo; I updated commit sha1. New commits:
8703f7f  fix snub cube example in polyhedra_tutorial.rst

comment:6 Changed 4 years ago by
Status:  needs_work → needs_review 

comment:7 Changed 4 years ago by
Status:  needs_review → positive_review 

comment:8 Changed 4 years ago by
Status:  positive_review → needs_work 

Fails its testsuite on Python 2.
Can't use 1/2
in the construction; it gives the float 0.5 (in the Python module, which uses from __future__ import division
)
comment:9 Changed 4 years ago by
(On branch public/25097/qnormalizalgebraic
I also have an improvement of the snub cube construction, which handles the backend parameter. Perhaps the code should be combined.)
comment:10 Changed 4 years ago by
Commit:  8703f7f85aa7b7f0161a8c90fa92dd0d2e47dc70 → 05867b434268870a94f105f7ba699a4dea0bbdf6 

Branch pushed to git repo; I updated commit sha1. New commits:
05867b4  Merge tag '8.8.beta3' into t/26340/public/26340

comment:11 Changed 4 years ago by
Commit:  05867b434268870a94f105f7ba699a4dea0bbdf6 → 39c3c67d67e61d1f4055ad41e095b9aaea4b346c 

Branch pushed to git repo; I updated commit sha1. New commits:
39c3c67  Fix construction of snub_cube data

comment:12 Changed 4 years ago by
Authors:  Laith Rastanawi → Laith Rastanawi, Matthias Koeppe 

Description:  modified (diff) 
Reviewers:  vincent Delecroix → Vincent Delecroix 
Status:  needs_work → needs_review 
Summary:  polytopes.snub_cube should be set up with base_ring=QQ → polytopes.snub_cube should be set up in exact arithmetic 
comment:13 followup: 14 Changed 4 years ago by
Considering how long it takes to construct the polytope with exact=True
, I think default should still be exact=False
. However, I do like that the exact option is there for those who want it.
comment:14 followup: 15 Changed 4 years ago by
Replying to tscrim:
Considering how long it takes to construct the polytope with
exact=True
, I think default should still beexact=False
. However, I do like that the exact option is there for those who want it.
The normaliz
backend will soon accept number fields as base ring and this computation will take much less time, see #25097.
Once it is possible, I suggest to specify the backend normaliz
with exact=True
in the test...
comment:15 followup: 17 Changed 4 years ago by
Replying to jipilab:
Replying to tscrim:
Considering how long it takes to construct the polytope with
exact=True
, I think default should still beexact=False
. However, I do like that the exact option is there for those who want it.The
normaliz
backend will soon accept number fields as base ring and this computation will take much less time, see #25097.Once it is possible, I suggest to specify the backend
normaliz
withexact=True
in the test...
I agree with this overall, but I would say you should have all 3 tests (exact=False
, exact=True
with # long time
, and exact=True
with # optional  normaliz
) for good coverage.
Once #25097, you could have exact=None
, which checks if normaliz
is installed and does the exact if it is and the inexact if not. However, for now, I would again suggest staying with exact=False
as the default.
comment:16 Changed 4 years ago by
Commit:  39c3c67d67e61d1f4055ad41e095b9aaea4b346c → e70d4a5f81041f942f22bc70e559ea1a7531eb12 

Branch pushed to git repo; I updated commit sha1. New commits:
e70d4a5  snub_cube: Change default back to exact=False

comment:17 Changed 4 years ago by
I agree with Travis and have changed it back to default exact=False
.
Replying to tscrim:
I agree with this overall, but I would say you should have all 3 tests (
exact=False
,exact=True
with# long time
, andexact=True
with# optional  normaliz
) for good coverage.
Yes, on #25097 we already have all 3 tests.
comment:18 Changed 4 years ago by
Description:  modified (diff) 

Reviewers:  Vincent Delecroix → Vincent Delecroix, JeanPhilippe Labbé, Travis Scrimshaw 
Summary:  polytopes.snub_cube should be set up in exact arithmetic → polytopes.snub_cube should allow exact coordinates 
Apart from the annoying convention in line
  ``exact``  (boolean, default ``False``) if ``True`` use exact +  ``exact``  (boolean, default ``False``) if ``True`` use exact
it looks good to go to me. If you fix this, you can set it to positive review on my behalf.
comment:19 Changed 4 years ago by
Commit:  e70d4a5f81041f942f22bc70e559ea1a7531eb12 → 13d8c44c9f73ac3f7033fc819781a6e349f1637a 

Branch pushed to git repo; I updated commit sha1. New commits:
13d8c44  docstring fix

comment:20 Changed 4 years ago by
Status:  needs_review → positive_review 

comment:21 Changed 4 years ago by
Branch:  public/26340 → 13d8c44c9f73ac3f7033fc819781a6e349f1637a 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
add exact option to snub_cube