Opened 2 years ago

Closed 2 years ago

#24835 closed enhancement (fixed)

Change error message in construction of polyhedron object

Reported by: jipilab Owned by:
Priority: major Milestone: sage-8.3
Component: geometry Keywords: days93, IMA-PolyGeom
Cc: vdelecroix, moritz Merged in:
Authors: Jean-Philippe Labbé Reviewers: Moritz Firsching
Report Upstream: N/A Work issues:
Branch: 7005c9e (Commits) Commit: 7005c9e2175cc04058f08b7c3cad01379b64a2c1
Dependencies: #22572 Stopgaps:

Description

The current error message given by

    sage: Polyhedron([[sqrt(2)]])
    Traceback (most recent call last):
    ...
    ValueError: for polyhedra with floating point numbers, the only allowed ring is RDF with backend 'cdd'

should be replaced by "ValueError?: the only allowed inexact ring is RDF with backend 'cdd'"

Change History (17)

comment:1 follow-ups: Changed 2 years ago by moritz

Wouldn't it be good, if it automatically detected AA? (Perhaps not..)

sage: P = Polyhedron([[sqrt(2)]], base_ring=AA)
sage: P.vertices()
(A vertex at (1.414213562373095?),)

comment:2 Changed 2 years ago by jipilab

  • Dependencies set to 22572

comment:3 Changed 2 years ago by moritz

  • Dependencies changed from 22572 to #22572

comment:4 in reply to: ↑ 1 Changed 2 years ago by vdelecroix

Replying to moritz:

Wouldn't it be good, if it automatically detected AA? (Perhaps not..)

sage: P = Polyhedron([[sqrt(2)]], base_ring=AA)
sage: P.vertices()
(A vertex at (1.414213562373095?),)

I don't want to special case input coming from the symbolic ring in the polyhedron code. This is the kind of thing that would have to be repeated everywhere. So either sqrt(2) should be a proper number field element (possibly AA) or this will stay as it is.

comment:5 in reply to: ↑ 1 Changed 2 years ago by jipilab

Replying to moritz:

Wouldn't it be good, if it automatically detected AA? (Perhaps not..)

sage: P = Polyhedron([[sqrt(2)]], base_ring=AA)
sage: P.vertices()
(A vertex at (1.414213562373095?),)

Forcing the base ring to be algebraic numbers is a bit "DWIM"... so I would allow it as it is above. If the user is lazy and still puts the base ring to be AA with square roots everywhere... it's still nice to be able to get what you want, but it should not allow to simply but symbolic stuff at all.

comment:6 Changed 2 years ago by jipilab

  • Branch set to u/jipilab/24835
  • Commit set to 84ec284c50cf936645f7befd6f50324e40e42e55

New commits:

18293d5initial commit
e1904c622572: first draft of polyhedra tutorial
5a09b6922572: revamp polyhedra tutorials
db5e413Updated to 8.2.b5, fixed many errors
62c5d56Merge branch 'public/22572v8.2b5' of trac.sagemath.org:sage into 24835
84ec284Fixed the tutorial

comment:7 Changed 2 years ago by jipilab

  • Status changed from new to needs_review

comment:8 Changed 2 years ago by git

  • Commit changed from 84ec284c50cf936645f7befd6f50324e40e42e55 to 85dc6f4abcbc8a02c3a643a9f46455c1018ef006

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

85dc6f4pep8

comment:9 Changed 2 years ago by git

  • Commit changed from 85dc6f4abcbc8a02c3a643a9f46455c1018ef006 to 5e2b8e5b26389acef88cfd256afb3eac1da7869a

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

5e2b8e5Merge branch 'develop' into 24835

comment:10 Changed 2 years ago by git

  • Commit changed from 5e2b8e5b26389acef88cfd256afb3eac1da7869a to b32299ac40239dd5dd98abf05682123ec5aa22b0

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

31edde6Completed a comment
231cd56deleted trailing whitespace
f685c64fixed some typos in lectures.rst
7f5e125fixed typos in polyhedra_quickref.rst
bd6c356LateX -> LaTeX in polytope_tikz.rst
b1ce45bSeveral other corrections
d526f70renamed tutorial files
d7896f8Merge branch 'develop' into 22572
597b802Merge branch sage8.2.rc1 into 22572
b32299aMerge branch '22572' into 24835

comment:11 Changed 2 years ago by moritz

  • Reviewers set to Moritz Firsching
  • Status changed from needs_review to positive_review

Looks good!

comment:12 Changed 2 years ago by git

  • Commit changed from b32299ac40239dd5dd98abf05682123ec5aa22b0 to 7005c9e2175cc04058f08b7c3cad01379b64a2c1
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

eee533aCorrections from review
b74ae85Made tests pass
7005c9eMerge branch '22572' into 24835

comment:13 Changed 2 years ago by jipilab

Merge the dependency.

comment:14 Changed 2 years ago by moritz

  • Status changed from needs_review to positive_review

comment:15 Changed 2 years ago by jipilab

  • Keywords IMA-PolyGeom added

comment:16 Changed 2 years ago by jipilab

  • Milestone changed from sage-8.2 to sage-8.3

comment:17 Changed 2 years ago by vbraun

  • Branch changed from u/jipilab/24835 to 7005c9e2175cc04058f08b7c3cad01379b64a2c1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.