Opened 3 years ago

Closed 3 years ago

#27840 closed defect (fixed)

Bug in the ppl backend of polyhedron

Reported by: jipilab Owned by:
Priority: major Milestone: sage-8.8
Component: geometry Keywords: polytopes, ppl
Cc: mkoeppe, vdelecroix, tscrim Merged in:
Authors: Matthias Koeppe Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 17895a6 (Commits, GitHub, GitLab) Commit: 17895a6256b490db70d3758eef2938417295980d
Dependencies: Stopgaps:

Status badges

Description

The following line throws a conversion error only with the (default) backend ppl: 

sage: Q = Polyhedron(backend='ppl', vertices=[(1, 2, 3), (1, 3, 2), (2, 1, 3), (2, 3, 1), (3, 1, 2), (3, 2, 1)],rays=[[1,1,1]],lines=[[1,2,3]])
Traceback (most recent call last):
...
TypeError: no conversion of this rational to integer

Replacing the backend by any other goes through without problems.

Change History (12)

comment:1 Changed 3 years ago by mkoeppe

Interesting. Here the heuristic that guesses the parent and base ring goes wrong. If one passes base_ring=QQ, then one can see that PPL chooses a representation using a fractional "vertex".

comment:2 Changed 3 years ago by vdelecroix

What should be done? Set base_ring=QQ when there is rays or lines?

Do you have any idea why PPL prefers a presentation with vertices with non-trivial denominators?

comment:3 Changed 3 years ago by mkoeppe

  • Branch set to u/mkoeppe/bug_in_the_ppl_backend_of_polyhedron

comment:4 Changed 3 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 40737833f88e5fc8f373954da89ef36ccfcf530c
  • Status changed from new to needs_review

New commits:

​4073783Guess base_ring of non-compact V-polyhedra with integer data as QQ, not ZZ

comment:5 Changed 3 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

Failures in 

sage -t --long src/sage/geometry/cone.py  # 3 doctests failed
sage -t --long src/sage/doctest/control.py  # 4 doctests failed
sage -t --long src/sage/doctest/forker.py  # 6 doctests failed
sage -t --long src/sage/combinat/root_system/plot.py  # 1 doctest failed

(see patchbot report)

I believe that when the polytope is a cone (namely with a unique vertex at 0) then everything should be fine. No? Don't we want base_ring=ZZ in this case?

Also, the constructor should be tested when the user enforces base_ring=ZZ.

comment:6 Changed 3 years ago by git

  • Commit changed from 40737833f88e5fc8f373954da89ef36ccfcf530c to a08167121554477f85e14cc2ce25819f87a4fb48

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

​cb66a1eHowever, guess base_ring of cones with integer data as ZZ
​a081671Add a doctest

comment:7 Changed 3 years ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:8 follow-up: Changed 3 years ago by vdelecroix

I like more this less intrusive version. Though it is a pity that the second commit cancels some edits of the first...

comment:9 Changed 3 years ago by git

  • Commit changed from a08167121554477f85e14cc2ce25819f87a4fb48 to 17895a6256b490db70d3758eef2938417295980d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​27b98b5Guess base_ring of non-compact, non-cone V-polyhedra with integer data as QQ, not ZZ
​17895a6Add a doctest

comment:10 in reply to: ↑ 8 Changed 3 years ago by mkoeppe

Replying to vdelecroix:

it is a pity that the second commit cancels some edits of the first...

I've squashed the two commits.

comment:11 Changed 3 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Nice Thx.

comment:12 Changed 3 years ago by vbraun

  • Branch changed from u/mkoeppe/bug_in_the_ppl_backend_of_polyhedron to 17895a6256b490db70d3758eef2938417295980d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.