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: |
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
comment:2 Changed 3 years ago by
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
- Branch set to u/mkoeppe/bug_in_the_ppl_backend_of_polyhedron
comment:4 Changed 3 years ago by
- Commit set to 40737833f88e5fc8f373954da89ef36ccfcf530c
- Status changed from new to needs_review
New commits:
4073783 | Guess base_ring of non-compact V-polyhedra with integer data as QQ, not ZZ
|
comment:5 Changed 3 years ago by
- 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
- Commit changed from 40737833f88e5fc8f373954da89ef36ccfcf530c to a08167121554477f85e14cc2ce25819f87a4fb48
comment:7 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:8 follow-up: ↓ 10 Changed 3 years ago by
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
- Commit changed from a08167121554477f85e14cc2ce25819f87a4fb48 to 17895a6256b490db70d3758eef2938417295980d
comment:10 in reply to: ↑ 8 Changed 3 years ago by
Replying to vdelecroix:
it is a pity that the second commit cancels some edits of the first...
I've squashed the two commits.
comment:12 Changed 3 years ago by
- 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
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".