Opened 3 years ago
Closed 3 years ago
#27987 closed defect (fixed)
CombinatorialPolyhedron improve initialization, remove bug for unbounded polyhedra
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  geometry  Keywords:  
Cc:  jipilab, tscrim, vdelecroix  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  806a217 (Commits, GitHub, GitLab)  Commit:  806a217944c9332ec3e09f153cbd589075d0fa0f 
Dependencies:  Stopgaps: 
Description (last modified by )
CombinatorialPolyhedron
as of #26887 only requires the number of lines (as n_lines
in Polyhedron_base
). This does not suffice, as
sage: P1 = Polyhedron(vertices=[[0,1],[1,0]], rays=[[1,1]]) sage: P2 = Polyhedron(vertices=[[0,1],[1,0],[1,1]]) sage: P1.incidence_matrix() == P2.incidence_matrix() True
Instead of just specifying the n_lines
, one should specify unbounded
and a far face.
Unfortunately, determining the rays from just the incidence matrix does not seem to work.
Now with the far face at hand anyway, it's much easier to determine the (combinatorial) vertices.
Change History (22)
comment:1 Changed 3 years ago by
 Summary changed from `CombinatorialPolyhedron` improve initialization, remove bug for unbounded polyhedra to CombinatorialPolyhedron improve initialization, remove bug for unbounded polyhedra
comment:2 Changed 3 years ago by
 Branch set to public/27987
 Commit set to 39810e5194daea168c01f21b8f8efaeb24f5bf15
comment:3 Changed 3 years ago by
 Milestone sage8.8 deleted
As the Sage8.8 release milestone is pending, we should delete the sage8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage8.9).
comment:4 Changed 3 years ago by
 Milestone set to sage8.9
comment:5 Changed 3 years ago by
There is also a python3 failing doctest:
This triggers a new failing doctest with python3sage:
sage t long src/sage/geometry/polyhedron/combinatorial_polyhedron/base.pyx ********************************************************************** File "src/sage/geometry/polyhedron/combinatorial_polyhedron/base.pyx", line 937, in sage.geometry.polyhedron.combinatorial_polyhedron.base.CombinatorialPolyhedron.edge_graph Failed example: G.degree() Expected: [4, 3, 4, 3, 4] Got: [3, 4, 4, 3, 4]
comment:6 Changed 3 years ago by
I have made #28153 for the py3 problem, please review.
comment:7 Changed 3 years ago by
 Branch changed from public/27987 to public/27987new
 Commit changed from 39810e5194daea168c01f21b8f8efaeb24f5bf15 to cdc1522e98cf0e17e88cb3fe0c4515f6e7925a51
New commits:
cdc1522  combinatorial polyhedron uses far face as a bug fix

comment:8 Changed 3 years ago by
 Commit changed from cdc1522e98cf0e17e88cb3fe0c4515f6e7925a51 to c2f175d6691caa21a03495844a621f468e1b5bfc
Branch pushed to git repo; I updated commit sha1. New commits:
c2f175d  small changes

comment:9 Changed 3 years ago by
 Commit changed from c2f175d6691caa21a03495844a621f468e1b5bfc to b90d25335cb2ac2ec91b08ed3b38993ffc60012d
Branch pushed to git repo; I updated commit sha1. New commits:
b90d253  more small changes

comment:10 Changed 3 years ago by
 Commit changed from b90d25335cb2ac2ec91b08ed3b38993ffc60012d to d6d663a56310c63283adc279e79ec4eb2e983581
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d6d663a  combinatorial polyhedron uses far face as a bug fix

comment:11 Changed 3 years ago by
 Commit changed from d6d663a56310c63283adc279e79ec4eb2e983581 to 14d17a87bb48e32c98083b974e9cebd033390d80
Branch pushed to git repo; I updated commit sha1. New commits:
14d17a8  added doctest reporting the bug fix

comment:12 Changed 3 years ago by
 Description modified (diff)
 Status changed from new to needs_review
New commits:
14d17a8  added doctest reporting the bug fix

comment:13 Changed 3 years ago by
The whole business with testing an interrupt is really unstable. It seems to work almost all of the time, but having the tests fail in even 1 percent of the tests is not acceptable, is it? (Imagine this being the case for all modules.)
I do not know, where I could possible have made a mistake with catching an interrupt. I suspect that it just sometimes fails at some place that I have no impact on.
Anyway, maybe I should remove those fragile tests (in a new ticket).
comment:14 Changed 3 years ago by
 Dependencies #26887 deleted
 Description modified (diff)
comment:15 Changed 3 years ago by
 Cc jipilab tscrim vdelecroix added
comment:16 followup: ↓ 17 Changed 3 years ago by
A few things:
Are you absolutely sure that nobody will (should) be passing the n_lines
data to the constructor (unless they know what they are doing)? If not, then you should deprecate that (including having it be in its original position).
#27987
> :trac:`12987`
if unbounded is False:
> if not unbounded:
(unless you expect unbounded
to possibly be something else like None
that should be treated differently, in which case you should add a comment about that).
comment:17 in reply to: ↑ 16 ; followup: ↓ 18 Changed 3 years ago by
Thanks.
Replying to tscrim:
A few things:
Are you absolutely sure that nobody will (should) be passing the
n_lines
data to the constructor (unless they know what they are doing)? If not, then you should deprecate that (including having it be in its original position).
CombinatorialPolyhedron
has not been in the master branch yet. So I figured it is ok. I can throw a warning, when unbounded is an integer, to make sure no one accidentally uses it.
At the moment, I'm forcing the user to use the Vrepresentation as in Polyhedron
w.r. to the lines in the representation.
Maybe this is to restrictive and I should allow the user to just pass the dimension of the affine space contained in the polyhedron instead (raising the dimension by that number).
#27987
>:trac:`12987`
if unbounded is False:
>if not unbounded:
(unless you expectunbounded
to possibly be something else likeNone
that should be treated differently, in which case you should add a comment about that).
comment:18 in reply to: ↑ 17 ; followup: ↓ 19 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
Replying to ghkliem:
Thanks.
Replying to tscrim:
A few things:
Are you absolutely sure that nobody will (should) be passing the
n_lines
data to the constructor (unless they know what they are doing)? If not, then you should deprecate that (including having it be in its original position).
CombinatorialPolyhedron
has not been in the master branch yet. So I figured it is ok. I can throw a warning, when unbounded is an integer, to make sure no one accidentally uses it.
Okay, then this is good as it currently is (i.e., you do not need to issue a warning).
At the moment, I'm forcing the user to use the Vrepresentation as in
Polyhedron
w.r. to the lines in the representation. Maybe this is to restrictive and I should allow the user to just pass the dimension of the affine space contained in the polyhedron instead (raising the dimension by that number).
That is probably best for another ticket.
So if you make the other two changes I mentioned, then you can set a positive review on my behalf.
comment:19 in reply to: ↑ 18 Changed 3 years ago by
Great. I will do so probably on Saturday (no PC at the moment).
Replying to tscrim:
Replying to ghkliem:
Thanks.
Replying to tscrim:
A few things:
Are you absolutely sure that nobody will (should) be passing the
n_lines
data to the constructor (unless they know what they are doing)? If not, then you should deprecate that (including having it be in its original position).
CombinatorialPolyhedron
has not been in the master branch yet. So I figured it is ok. I can throw a warning, when unbounded is an integer, to make sure no one accidentally uses it.Okay, then this is good as it currently is (i.e., you do not need to issue a warning).
At the moment, I'm forcing the user to use the Vrepresentation as in
Polyhedron
w.r. to the lines in the representation. Maybe this is to restrictive and I should allow the user to just pass the dimension of the affine space contained in the polyhedron instead (raising the dimension by that number).That is probably best for another ticket.
So if you make the other two changes I mentioned, then you can set a positive review on my behalf.
comment:20 Changed 3 years ago by
 Commit changed from 14d17a87bb48e32c98083b974e9cebd033390d80 to 806a217944c9332ec3e09f153cbd589075d0fa0f
Branch pushed to git repo; I updated commit sha1. New commits:
806a217  correct link to trac ticket, if unbounded is False > if not unbounded

comment:21 Changed 3 years ago by
 Status changed from needs_review to positive_review
Two changes mentioned by Travis taken care of.
comment:22 Changed 3 years ago by
 Branch changed from public/27987new to 806a217944c9332ec3e09f153cbd589075d0fa0f
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
added documentation and examples to each module
correct hyperlinks
documentation fix
Do not iterate twice for CombinatorialPolyhedron.facets()
added combinatorial face
improved docstring in list_of_all_faces
fixed small issues
A number of small edits.
Merge branch 'public/26887' of git://trac.sagemath.org/sage into develop
changed input of combintorial polyhedron