Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

is_pyramid returns a wrong certificate #30292

Closed
LaisRast opened this issue Aug 5, 2020 · 11 comments
Closed

is_pyramid returns a wrong certificate #30292

LaisRast opened this issue Aug 5, 2020 · 11 comments

Comments

@LaisRast
Copy link

LaisRast commented Aug 5, 2020

The method is_pyramid of CombinatorialPolyhedron returns a wrong certificate:

sage: Polyhedron([[0, -1, -1], [0, -1, 1], [0, 1, -1], [0, 1, 1], [1, 0, 0]]).is_pyramid(certificate=True)
(True, A vertex at (0, -1, -1))

This bug introduced in #29189; the indexing in _face_iter(True, 0) is different from the indexing in Vrepresentation.

CC: @jplab @kliem

Component: geometry

Keywords: polytope, combinatorialpolyhedron, is_pyramid

Author: Jonathan Kliem

Branch/Commit: 4c5c730

Reviewer: Matthias Koeppe

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

@LaisRast LaisRast added this to the sage-9.2 milestone Aug 5, 2020
@kliem
Copy link
Contributor

kliem commented Aug 10, 2020

comment:1

Thanks for catching this. Stupid mistake.

@kliem
Copy link
Contributor

kliem commented Aug 10, 2020

New commits:

fb22b48fix certificate for pyramid

@kliem
Copy link
Contributor

kliem commented Aug 10, 2020

Branch: public/30292

@kliem
Copy link
Contributor

kliem commented Aug 10, 2020

Author: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Aug 10, 2020

Commit: fb22b48

@mkoeppe
Copy link
Member

mkoeppe commented Aug 10, 2020

comment:3

Perhaps there should be a _test... method that checks the certificate?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2020

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

4c5c730add method `_test_pyramid`

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2020

Changed commit from fb22b48 to 4c5c730

@mkoeppe
Copy link
Member

mkoeppe commented Aug 10, 2020

Reviewer: Matthias Koeppe

@kliem
Copy link
Contributor

kliem commented Aug 11, 2020

comment:7

Thank you.

@vbraun
Copy link
Member

vbraun commented Aug 12, 2020

Changed branch from public/30292 to 4c5c730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants