#27987 closed defect (fixed)

CombinatorialPolyhedron improve initialization, remove bug for unbounded polyhedra

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description (last modified by gh-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.

Change History (22)

comment:1 Changed 22 months ago by gh-kliem

  • Summary changed from `CombinatorialPolyhedron` improve initialization, remove bug for unbounded polyhedra to CombinatorialPolyhedron improve initialization, remove bug for unbounded polyhedra

comment:2 Changed 22 months ago by gh-kliem

  • Branch set to public/27987
  • Commit set to 39810e5194daea168c01f21b8f8efaeb24f5bf15

Last 10 new commits:

9b69f50added documentation and examples to each module
4e8fd8ccorrect hyperlinks
72ac3b0documentation fix
611099fDo not iterate twice for CombinatorialPolyhedron.facets()
d38e130added combinatorial face
ca60665improved docstring in list_of_all_faces
abe00b6fixed small issues
8765313A number of small edits.
05ffcccMerge branch 'public/26887' of git://trac.sagemath.org/sage into develop
39810e5changed input of combintorial polyhedron

comment:3 Changed 22 months ago by embray

  • Milestone sage-8.8 deleted

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

comment:4 Changed 22 months ago by gh-kliem

  • Milestone set to sage-8.9

comment:5 Changed 21 months ago by chapoton

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]

comment:6 Changed 21 months ago by chapoton

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

comment:7 Changed 21 months ago by gh-kliem

  • Branch changed from public/27987 to public/27987-new
  • Commit changed from 39810e5194daea168c01f21b8f8efaeb24f5bf15 to cdc1522e98cf0e17e88cb3fe0c4515f6e7925a51

New commits:

cdc1522combinatorial polyhedron uses far face as a bug fix

comment:8 Changed 21 months ago by git

  • Commit changed from cdc1522e98cf0e17e88cb3fe0c4515f6e7925a51 to c2f175d6691caa21a03495844a621f468e1b5bfc

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

c2f175dsmall changes

comment:9 Changed 21 months ago by git

  • Commit changed from c2f175d6691caa21a03495844a621f468e1b5bfc to b90d25335cb2ac2ec91b08ed3b38993ffc60012d

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

b90d253more small changes

comment:10 Changed 21 months ago by git

  • Commit changed from b90d25335cb2ac2ec91b08ed3b38993ffc60012d to d6d663a56310c63283adc279e79ec4eb2e983581

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

d6d663acombinatorial polyhedron uses far face as a bug fix

comment:11 Changed 21 months ago by git

  • Commit changed from d6d663a56310c63283adc279e79ec4eb2e983581 to 14d17a87bb48e32c98083b974e9cebd033390d80

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

14d17a8added doctest reporting the bug fix

comment:12 Changed 21 months ago by gh-kliem

  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

14d17a8added doctest reporting the bug fix

comment:13 Changed 21 months ago by gh-kliem

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

comment:14 Changed 21 months ago by gh-kliem

  • Dependencies #26887 deleted
  • Description modified (diff)

comment:15 Changed 21 months ago by gh-kliem

  • Cc jipilab tscrim vdelecroix added

comment:16 follow-up: Changed 21 months ago by 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).

#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 ; follow-up: Changed 21 months ago by gh-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.

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

comment:18 in reply to: ↑ 17 ; follow-up: Changed 21 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

Replying to gh-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.

comment:19 in reply to: ↑ 18 Changed 21 months ago by gh-kliem

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

Replying to tscrim:

Replying to gh-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.

comment:20 Changed 21 months ago by git

  • Commit changed from 14d17a87bb48e32c98083b974e9cebd033390d80 to 806a217944c9332ec3e09f153cbd589075d0fa0f

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

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

comment:21 Changed 21 months ago by gh-kliem

  • Status changed from needs_review to positive_review

Two changes mentioned by Travis taken care of.

comment:22 Changed 21 months ago by vbraun

  • Branch changed from public/27987-new to 806a217944c9332ec3e09f153cbd589075d0fa0f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.