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: sage-8.8
Component: geometry Keywords: snub_cube, polytopes
Cc: Nico Van Cleemput, Jean-Philippe Labbé, Laith Rastanawi Merged in:
Authors: Laith Rastanawi, Matthias Koeppe Reviewers: Vincent Delecroix, Jean-Philippe Labbé, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 13d8c44 (Commits, GitHub, GitLab) Commit: 13d8c44c9f73ac3f7033fc819781a6e349f1637a
Dependencies: Stopgaps:

Status badges

Description (last modified by Jean-Philippe Labbé)

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 Jean-Philippe Labbé

Cc: Laith Rastanawi added

comment:2 Changed 4 years ago by Laith Rastanawi

Branch: public/26340
Commit: 57a9d108994a33efedfaf4b6d6732358db882800
Keywords: snub_cube polytopes added
Milestone: sage-8.4sage-8.8
Status: newneeds_review

New commits:

57a9d10add exact option to snub_cube

comment:3 Changed 4 years ago by Laith Rastanawi

Authors: Laith Rastanawi

comment:4 Changed 4 years ago by Vincent Delecroix

Reviewers: vincent Delecroix
Status: needs_reviewneeds_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 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
----------------------------------------------------------------------

comment:5 Changed 4 years ago by git

Commit: 57a9d108994a33efedfaf4b6d6732358db8828008703f7f85aa7b7f0161a8c90fa92dd0d2e47dc70

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

8703f7ffix snub cube example in polyhedra_tutorial.rst

comment:6 Changed 4 years ago by Laith Rastanawi

Status: needs_workneeds_review

comment:7 Changed 4 years ago by Vincent Delecroix

Status: needs_reviewpositive_review

comment:8 Changed 4 years ago by Matthias Köppe

Status: positive_reviewneeds_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 Matthias Köppe

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

comment:10 Changed 4 years ago by git

Commit: 8703f7f85aa7b7f0161a8c90fa92dd0d2e47dc7005867b434268870a94f105f7ba699a4dea0bbdf6

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

05867b4Merge tag '8.8.beta3' into t/26340/public/26340

comment:11 Changed 4 years ago by git

Commit: 05867b434268870a94f105f7ba699a4dea0bbdf639c3c67d67e61d1f4055ad41e095b9aaea4b346c

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

39c3c67Fix construction of snub_cube data

comment:12 Changed 4 years ago by Matthias Köppe

Authors: Laith RastanawiLaith Rastanawi, Matthias Koeppe
Description: modified (diff)
Reviewers: vincent DelecroixVincent Delecroix
Status: needs_workneeds_review
Summary: polytopes.snub_cube should be set up with base_ring=QQpolytopes.snub_cube should be set up in exact arithmetic

comment:13 Changed 4 years ago by Travis Scrimshaw

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 ; Changed 4 years ago by Jean-Philippe Labbé

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

comment:15 in reply to:  14 ; Changed 4 years ago by Travis Scrimshaw

Replying to jipilab:

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.

comment:16 Changed 4 years ago by git

Commit: 39c3c67d67e61d1f4055ad41e095b9aaea4b346ce70d4a5f81041f942f22bc70e559ea1a7531eb12

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

e70d4a5snub_cube: Change default back to exact=False

comment:17 in reply to:  15 Changed 4 years ago by Matthias Köppe

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.

comment:18 Changed 4 years ago by Jean-Philippe Labbé

Description: modified (diff)
Reviewers: Vincent DelecroixVincent Delecroix, Jean-Philippe Labbé, Travis Scrimshaw
Summary: polytopes.snub_cube should be set up in exact arithmeticpolytopes.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.

Last edited 4 years ago by Jean-Philippe Labbé (previous) (diff)

comment:19 Changed 4 years ago by git

Commit: e70d4a5f81041f942f22bc70e559ea1a7531eb1213d8c44c9f73ac3f7033fc819781a6e349f1637a

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

13d8c44docstring fix

comment:20 Changed 4 years ago by Matthias Köppe

Status: needs_reviewpositive_review

comment:21 Changed 4 years ago by Volker Braun

Branch: public/2634013d8c44c9f73ac3f7033fc819781a6e349f1637a
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.