polytopes.snub_cube should allow exact coordinates

[mkoeppe]

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.

CC: @nvcleemp @jplab @LaisRast

Component: geometry

Keywords: snub_cube, polytopes

Author: Laith Rastanawi, Matthias Koeppe

Branch/Commit: 13d8c44

Reviewer: Vincent Delecroix, Jean-Philippe Labbé, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/26340

[LaisRast]

Changed keywords from none to snub_cube, polytopes

[LaisRast]

Commit: 57a9d10

[LaisRast]

Branch: public/26340

[LaisRast]

New commits:

57a9d10

add exact option to snub_cube

[LaisRast]

Author: Laith Rastanawi

[videlec]

Reviewer: vincent Delecroix

[videlec]

comment:4

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 3-dimensional polyhedron in RDF^3 defined as the convex hull of 24 vertices
Got:
    A 3-dimensional 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
----------------------------------------------------------------------

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 57a9d10 to 8703f7f

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

8703f7f

fix snub cube example in polyhedra_tutorial.rst

[mkoeppe]

comment:8

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 fromfutureimport division)

[mkoeppe]

comment:9

(On branch public/25097/qnormaliz-algebraic I also have an improvement of the snub cube construction, which handles the backend parameter. Perhaps the code should be combined.)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

05867b4

Merge tag '8.8.beta3' into t/26340/public/26340

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 8703f7f to 05867b4

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

39c3c67

Fix construction of snub_cube data

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 05867b4 to 39c3c67

[mkoeppe]

Changed reviewer from vincent Delecroix to Vincent Delecroix

[mkoeppe]

Changed author from Laith Rastanawi to Laith Rastanawi, Matthias Koeppe

[mkoeppe]

Description changed:

--- 
+++ 
@@ -1,4 +1,2 @@
-`polytopes.snub_cube` uses a scaling that makes this polytope rational (though calculations of the vertices has to go through a field extension). See #25097.
-But in contrast to other rational polytopes from the library, it is set up in RDF by default. 
-This should be changed.
+`polytopes.snub_cube` should be set up with a number field by default.

[tscrim]

comment:13

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.

[jplab]

comment:14

Replying to @tscrim:

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.

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...

[tscrim]

comment:15

Replying to @jplab:

Replying to @tscrim:

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.

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...

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.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

e70d4a5

snub_cube: Change default back to exact=False

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 39c3c67 to e70d4a5

[mkoeppe]

comment:17

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, and exact=True with # optional - normaliz) for good coverage.

Yes, on #25097 we already have all 3 tests.

[jplab]

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Jean-Philippe Labbé, Travis Scrimshaw

[jplab]

Description changed:

--- 
+++ 
@@ -1,2 +1,3 @@
-`polytopes.snub_cube` should be set up with a number field by default.
+`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`.

[jplab]

comment:18

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.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from e70d4a5 to 13d8c44

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

13d8c44

docstring fix

[vbraun]

Changed branch from public/26340 to 13d8c44