Bug in the ppl backend of polyhedron

[jplab]

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.

CC: @mkoeppe @videlec @tscrim

Component: geometry

Keywords: polytopes, ppl

Author: Matthias Koeppe

Branch/Commit: 17895a6

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/27840

[mkoeppe]

comment:1

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".

[videlec]

comment:2

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?

[mkoeppe]

Branch: u/mkoeppe/bug_in_the_ppl_backend_of_polyhedron

[mkoeppe]

Author: Matthias Koeppe

[mkoeppe]

Commit: 4073783

[mkoeppe]

New commits:

4073783

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

[videlec]

Reviewer: Vincent Delecroix

[videlec]

comment:5

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.

[sagetrac-git]

Changed commit from 4073783 to a081671

[sagetrac-git]

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

cb66a1e	However, guess base_ring of cones with integer data as ZZ
a081671	Add a doctest

[videlec]

comment:8

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

[sagetrac-git]

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

27b98b5	Guess base_ring of non-compact, non-cone V-polyhedra with integer data as QQ, not ZZ
17895a6	Add a doctest

[sagetrac-git]

Changed commit from a081671 to 17895a6

[mkoeppe]

comment:10

Replying to @videlec:

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

I've squashed the two commits.

[videlec]

comment:11

Nice Thx.

[vbraun]

Changed branch from u/mkoeppe/bug_in_the_ppl_backend_of_polyhedron to 17895a6