Opened 3 years ago

Last modified 3 years ago

#27987 closed defect

CombinatorialPolyhedron improve initialization, remove bug for unbounded polyhedra — at Version 14

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-8.9
Component: geometry Keywords:
Cc: Jean-Philippe Labbé, Travis Scrimshaw, Vincent Delecroix Merged in:
Authors: Jonathan Kliem Reviewers:
Report Upstream: N/A Work issues:
Branch: public/27987-new (Commits, GitHub, GitLab) Commit: 14d17a87bb48e32c98083b974e9cebd033390d80
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 (14)

comment:1 Changed 3 years ago by gh-kliem

Summary: `CombinatorialPolyhedron` improve initialization, remove bug for unbounded polyhedraCombinatorialPolyhedron improve initialization, remove bug for unbounded polyhedra

comment:2 Changed 3 years ago by gh-kliem

Branch: public/27987
Commit: 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 3 years ago by Erik Bray

Milestone: sage-8.8

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 3 years ago by gh-kliem

Milestone: sage-8.9

comment:5 Changed 3 years ago by Frédéric 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 3 years ago by Frédéric Chapoton

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

comment:7 Changed 3 years ago by gh-kliem

Branch: public/27987public/27987-new
Commit: 39810e5194daea168c01f21b8f8efaeb24f5bf15cdc1522e98cf0e17e88cb3fe0c4515f6e7925a51

New commits:

cdc1522combinatorial polyhedron uses far face as a bug fix

comment:8 Changed 3 years ago by git

Commit: cdc1522e98cf0e17e88cb3fe0c4515f6e7925a51c2f175d6691caa21a03495844a621f468e1b5bfc

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

c2f175dsmall changes

comment:9 Changed 3 years ago by git

Commit: c2f175d6691caa21a03495844a621f468e1b5bfcb90d25335cb2ac2ec91b08ed3b38993ffc60012d

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

b90d253more small changes

comment:10 Changed 3 years ago by git

Commit: b90d25335cb2ac2ec91b08ed3b38993ffc60012dd6d663a56310c63283adc279e79ec4eb2e983581

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 3 years ago by git

Commit: d6d663a56310c63283adc279e79ec4eb2e98358114d17a87bb48e32c98083b974e9cebd033390d80

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

14d17a8added doctest reporting the bug fix

comment:12 Changed 3 years ago by gh-kliem

Description: modified (diff)
Status: newneeds_review

New commits:

14d17a8added doctest reporting the bug fix

comment:13 Changed 3 years 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 3 years ago by gh-kliem

Dependencies: #26887
Description: modified (diff)
Note: See TracTickets for help on using tickets.