CombinatorialPolyhedron improve initialization, remove bug for unbounded polyhedra

[kliem]

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.

CC: @jplab @tscrim @videlec

Component: geometry

Author: Jonathan Kliem

Branch/Commit: 806a217

Reviewer: Travis Scrimshaw

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

[kliem]

Last 10 new commits:

9b69f50	added documentation and examples to each module
4e8fd8c	correct hyperlinks
72ac3b0	documentation fix
611099f	Do not iterate twice for CombinatorialPolyhedron.facets()
d38e130	added combinatorial face
ca60665	improved docstring in list_of_all_faces
abe00b6	fixed small issues
8765313	A number of small edits.
05ffccc	Merge branch 'public/26887' of git://trac.sagemath.org/sage into develop
39810e5	changed input of combintorial polyhedron

[kliem]

Branch: public/27987

[kliem]

Commit: 39810e5

[embray]

comment:3

As the Sage-8.8 release milestone is pending, we should delete the sage-8.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 (sage-8.9).

[fchapoton]

comment:5

There is also a python3 failing doctest:

This triggers a new failing doctest with python3-sage:

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]

[fchapoton]

comment:6

I have made #28153 for the py3 problem, please review.

[kliem]

Changed commit from 39810e5 to cdc1522

[kliem]

Changed branch from public/27987 to public/27987-new

[kliem]

New commits:

cdc1522

combinatorial polyhedron uses far face as a bug fix

[sagetrac-git]

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

c2f175d

small changes

[sagetrac-git]

Changed commit from cdc1522 to c2f175d

[sagetrac-git]

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

b90d253

more small changes

[sagetrac-git]

Changed commit from c2f175d to b90d253

[sagetrac-git]

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

[sagetrac-git]

Changed commit from b90d253 to d6d663a

[sagetrac-git]

Changed commit from d6d663a to 14d17a8

[sagetrac-git]

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

14d17a8

added doctest reporting the bug fix

[kliem]

New commits:

14d17a8

added doctest reporting the bug fix

[kliem]

comment:13

https://patchbot.sagemath.org/log/27987/debian/9.9/x86_64/4.9.0-9-amd64/cofio/2019-07-12%2013:08:53?short

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

[kliem]

Changed dependencies from #26887 to none

[tscrim]

comment:16

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

[kliem]

comment:17

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 expect unbounded to possibly be something else like None that should be treated differently, in which case you should add a comment about that).

[tscrim]

comment:18

Replying to @kliem:

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.

[tscrim]

Reviewer: Travis Scrimshaw

[kliem]

comment:19

Great. I will do so probably on Saturday (no PC at the moment).

Replying to @tscrim:

Replying to @kliem:

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.

[sagetrac-git]

Changed commit from 14d17a8 to 806a217

[sagetrac-git]

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

806a217

correct link to trac ticket, if unbounded is False -> if not unbounded

[kliem]

comment:21

Two changes mentioned by Travis taken care of.

[vbraun]

Changed branch from public/27987-new to 806a217