Opened 3 years ago
Closed 2 years ago
#26340 closed enhancement (fixed)
polytopes.snub_cube should allow exact coordinates
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  geometry  Keywords:  snub_cube, polytopes 
Cc:  nvcleemp, jipilab, ghLaisRast  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 2 years ago by
 Cc ghLaisRast added
comment:2 Changed 2 years ago by
 Branch set to public/26340
 Commit set to 57a9d108994a33efedfaf4b6d6732358db882800
 Keywords snub_cube polytopes added
 Milestone changed from sage8.4 to sage8.8
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
comment:4 Changed 2 years ago by
 Reviewers set to vincent Delecroix
 Status changed from needs_review to 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 2 years ago by
 Commit changed from 57a9d108994a33efedfaf4b6d6732358db882800 to 8703f7f85aa7b7f0161a8c90fa92dd0d2e47dc70
Branch pushed to git repo; I updated commit sha1. New commits:
8703f7f  fix snub cube example in polyhedra_tutorial.rst

comment:6 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:7 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:8 Changed 2 years ago by
 Status changed from positive_review to 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 2 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 2 years ago by
 Commit changed from 8703f7f85aa7b7f0161a8c90fa92dd0d2e47dc70 to 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 2 years ago by
 Commit changed from 05867b434268870a94f105f7ba699a4dea0bbdf6 to 39c3c67d67e61d1f4055ad41e095b9aaea4b346c
Branch pushed to git repo; I updated commit sha1. New commits:
39c3c67  Fix construction of snub_cube data

comment:12 Changed 2 years ago by
 Description modified (diff)
 Reviewers changed from vincent Delecroix to Vincent Delecroix
 Status changed from needs_work to needs_review
 Summary changed from polytopes.snub_cube should be set up with base_ring=QQ to polytopes.snub_cube should be set up in exact arithmetic
comment:13 followup: ↓ 14 Changed 2 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 in reply to: ↑ 13 ; followup: ↓ 15 Changed 2 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 in reply to: ↑ 14 ; followup: ↓ 17 Changed 2 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 2 years ago by
 Commit changed from 39c3c67d67e61d1f4055ad41e095b9aaea4b346c to e70d4a5f81041f942f22bc70e559ea1a7531eb12
Branch pushed to git repo; I updated commit sha1. New commits:
e70d4a5  snub_cube: Change default back to exact=False

comment:17 in reply to: ↑ 15 Changed 2 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 2 years ago by
 Description modified (diff)
 Reviewers changed from Vincent Delecroix to Vincent Delecroix, JeanPhilippe Labbé, Travis Scrimshaw
 Summary changed from polytopes.snub_cube should be set up in exact arithmetic to 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 2 years ago by
 Commit changed from e70d4a5f81041f942f22bc70e559ea1a7531eb12 to 13d8c44c9f73ac3f7033fc819781a6e349f1637a
Branch pushed to git repo; I updated commit sha1. New commits:
13d8c44  docstring fix

comment:20 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:21 Changed 2 years ago by
 Branch changed from public/26340 to 13d8c44c9f73ac3f7033fc819781a6e349f1637a
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
add exact option to snub_cube