Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#26435 closed enhancement (fixed)

polytopes.simplex: Add base_ring keyword, allow exact computation with project=True

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-8.5
Component: geometry Keywords:
Cc: jipilab, tscrim Merged in:
Authors: Matthias Koeppe Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: cecf0df (Commits) Commit: cecf0df7ae4eda4ad316e5b7e769cc58bc295425
Dependencies: Stopgaps:

Description

This enables setting up a regular simplex with project=True in algebraic reals

            sage: s3 = polytopes.simplex(3, project=True, base_ring=AA)
            sage: s3.volume() == sqrt(3+1) / factorial(3)
            True

Change History (9)

comment:1 Changed 2 years ago by mkoeppe

  • Branch set to u/mkoeppe/more_algebraic_polytopes

comment:2 Changed 2 years ago by mkoeppe

  • Cc jipilab tscrim added
  • Commit set to fc458d665ec05e49c217905abb908ebbdfb2e2d8
  • Status changed from new to needs_review

New commits:

fcd2752zero_sum_projection, project_points: Add keyword base_ring
fc458d6polytopes.simplex: Add base_ring keyword, allows exact computation with project=True

comment:3 Changed 2 years ago by tscrim

Why not just do

-    base_ring = kwds.pop('base_ring', None)
-    if base_ring is None:
-        base_ring = RDF
+    base_ring = kwds.pop('base_ring', RDF)

Otherwise LGTM.

comment:4 follow-up: Changed 2 years ago by mkoeppe

simplex passes through base_ring=None and delegates default handling to project_points in this way.

comment:5 in reply to: ↑ 4 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Replying to mkoeppe:

simplex passes through base_ring=None and delegates default handling to project_points in this way.

Ah, I see. Can you add a comment in the code explaining this so someone (like me) is not tempted to do the change I suggested in comment:3? Once done, you can set a positive review on my behalf.

comment:6 Changed 2 years ago by git

  • Commit changed from fc458d665ec05e49c217905abb908ebbdfb2e2d8 to cecf0df7ae4eda4ad316e5b7e769cc58bc295425

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

cecf0dfproject_points: Add documentation on default values of keyword

comment:7 Changed 2 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:8 Changed 2 years ago by vbraun

  • Branch changed from u/mkoeppe/more_algebraic_polytopes to cecf0df7ae4eda4ad316e5b7e769cc58bc295425
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:9 Changed 2 years ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

This should be re-targeted for 8.5.

Note: See TracTickets for help on using tickets.