#26340 closed enhancement (fixed)

polytopes.snub_cube should allow exact coordinates

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-8.8
Component: geometry Keywords: snub_cube, polytopes
Cc: nvcleemp, jipilab, gh-LaisRast Merged in:
Authors: Laith Rastanawi, Matthias Koeppe Reviewers: Vincent Delecroix, Jean-Philippe Labbé, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 13d8c44 (Commits) Commit: 13d8c44c9f73ac3f7033fc819781a6e349f1637a
Dependencies: Stopgaps:

Description (last modified by jipilab)

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 13 months ago by jipilab

  • Cc gh-LaisRast added

comment:2 Changed 12 months ago by gh-LaisRast

  • Branch set to public/26340
  • Commit set to 57a9d108994a33efedfaf4b6d6732358db882800
  • Keywords snub_cube polytopes added
  • Milestone changed from sage-8.4 to sage-8.8
  • Status changed from new to needs_review

New commits:

57a9d10add exact option to snub_cube

comment:3 Changed 12 months ago by gh-LaisRast

  • Authors set to Laith Rastanawi

comment:4 Changed 12 months ago by vdelecroix

  • 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 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 12 months ago by git

  • Commit changed from 57a9d108994a33efedfaf4b6d6732358db882800 to 8703f7f85aa7b7f0161a8c90fa92dd0d2e47dc70

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

8703f7ffix snub cube example in polyhedra_tutorial.rst

comment:6 Changed 12 months ago by gh-LaisRast

  • Status changed from needs_work to needs_review

comment:7 Changed 12 months ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:8 Changed 12 months ago by mkoeppe

  • 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 12 months ago by mkoeppe

(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 12 months ago by git

  • Commit changed from 8703f7f85aa7b7f0161a8c90fa92dd0d2e47dc70 to 05867b434268870a94f105f7ba699a4dea0bbdf6

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

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

comment:11 Changed 12 months ago by git

  • Commit changed from 05867b434268870a94f105f7ba699a4dea0bbdf6 to 39c3c67d67e61d1f4055ad41e095b9aaea4b346c

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

39c3c67Fix construction of snub_cube data

comment:12 Changed 12 months ago by mkoeppe

  • Authors changed from Laith Rastanawi to Laith Rastanawi, Matthias Koeppe
  • 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 follow-up: Changed 12 months ago by 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.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 12 months ago by 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...

comment:15 in reply to: ↑ 14 ; follow-up: Changed 12 months ago by tscrim

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 12 months ago by git

  • Commit changed from 39c3c67d67e61d1f4055ad41e095b9aaea4b346c to e70d4a5f81041f942f22bc70e559ea1a7531eb12

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 12 months ago by mkoeppe

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 12 months ago by jipilab

  • Description modified (diff)
  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, Jean-Philippe 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.

Last edited 12 months ago by jipilab (previous) (diff)

comment:19 Changed 12 months ago by git

  • Commit changed from e70d4a5f81041f942f22bc70e559ea1a7531eb12 to 13d8c44c9f73ac3f7033fc819781a6e349f1637a

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

13d8c44docstring fix

comment:20 Changed 12 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:21 Changed 11 months ago by vbraun

  • Branch changed from public/26340 to 13d8c44c9f73ac3f7033fc819781a6e349f1637a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.