Opened 10 years ago
Closed 10 years ago
#14479 closed defect (fixed)
Catch CDD errors during polyhedron construction
Reported by: | vbraun | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | geometry | Keywords: | |
Cc: | Merged in: | sage-5.10.beta1 | |
Authors: | Volker Braun | Reviewers: | Dmitrii Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
As reported on https://groups.google.com/d/msg/sage-support/lsUODuV47kc/7O0CLf1_ywMJ, numerically unstable input is not treated correctly in CDD and it errors out instead. This patch detects the error and converts it into a ValueError
Attachments (2)
Change History (12)
Changed 10 years ago by
Attachment: | trac_14479_cdd_error.patch added |
---|
comment:1 Changed 10 years ago by
Authors: | → Volker Braun |
---|---|
Status: | new → needs_review |
comment:2 Changed 10 years ago by
Could you also add in the doctest
Polyhedron(point_list,base_ring=QQ)
and
Polyhedron(point_list,backend='ppl',base_ring=QQ)
to give people more hints how to avoid that error?
By the way, is there any reason for having cdd
as the default backend? On this example it's more than 10 times slower than ppl
.
comment:3 Changed 10 years ago by
HMmmmmmmm... And isn't there something wrong with the "verbose" keywords you add to "init_from_Vrepresentation" and "init_from_Hrepresentation" ? You add them to the method, but you don't use them in the code that follows ! O_o
Nathann
comment:4 follow-up: 5 Changed 10 years ago by
cdd is the only backend for floating-point numbers, and therefore the default. Does anybody know a better floating-point implementation?
The verbose
keyword argument just wasn't patched through correctly.
comment:5 Changed 10 years ago by
Replying to vbraun:
cdd is the only backend for floating-point numbers, and therefore the default. Does anybody know a better floating-point implementation?
The
verbose
keyword argument just wasn't patched through correctly.
do you mean to say that something needs to be fixed in your patch?
comment:6 follow-up: 7 Changed 10 years ago by
Reviewers: | → Dmitrii Pasechnik |
---|---|
Status: | needs_review → positive_review |
No, I was just trying to explain why my patch adds the verbose
keyword argument in some places.
Positive review to the reviewer patch.
comment:7 Changed 10 years ago by
Description: | modified (diff) |
---|
Replying to vbraun:
No, I was just trying to explain why my patch adds the
verbose
keyword argument in some places.Positive review to the reviewer patch.
all looks good to me, positive review.
comment:8 Changed 10 years ago by
What I don't get is why you would add a verbose
arguments to methods that never use the verbose
variable in their code O_o
Nathann
comment:9 Changed 10 years ago by
Both Polyhedron_ppl
and Polyhedron_cdd
derive from Polyhedron_base. They both override
_init_from_Vrepresentation. One of them doesn't (yet) do anything if you pass
verbose=True`, but it should still accept the argument so that you can use the two derived classes interchangeably.
comment:10 Changed 10 years ago by
Merged in: | → sage-5.10.beta1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Initial patch