Opened 8 years ago

Closed 8 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:

Status badges

Description (last modified by dimpase)

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)

trac_14479_cdd_error.patch (4.9 KB) - added by vbraun 8 years ago.
Initial patch
trac_14479_reviewer.patch (817 bytes) - added by dimpase 8 years ago.
reviewer patch

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by vbraun

Initial patch

comment:1 Changed 8 years ago by vbraun

  • Authors set to Volker Braun
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by dimpase

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 8 years ago by ncohen

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: Changed 8 years ago by 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.

Changed 8 years ago by dimpase

reviewer patch

comment:5 in reply to: ↑ 4 Changed 8 years ago by dimpase

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: Changed 8 years ago by vbraun

  • Reviewers set to Dmitrii Pasechnik
  • Status changed from needs_review to 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 in reply to: ↑ 6 Changed 8 years ago by dimpase

  • 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 8 years ago by ncohen

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 8 years ago by vbraun

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 8 years ago by jdemeyer

  • Merged in set to sage-5.10.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.