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

Migrate is_lawrence_polytope and is_pyramid to combinatorial polyhedron #29189

Closed
kliem opened this issue Feb 12, 2020 · 24 comments
Closed

Migrate is_lawrence_polytope and is_pyramid to combinatorial polyhedron #29189

kliem opened this issue Feb 12, 2020 · 24 comments

Comments

@kliem
Copy link
Contributor

kliem commented Feb 12, 2020

This ticket migrates the methods is_lawrence_polytope and is_pyramid from Polyhedron_base to CombinatorialPolyhedron.

Also, we change the output for the 0-dimensional polyhedron. It is a pyramid over the empty polyhedron, even if it is not constructable in sage.

Along the way we fix a small bug, where the trivial combinatorial polyhedron in dimension 0 was set up without vertices (and facets). The bug fix is tested by CombinatorialPolyhedron(0).is_pyramid(certificate=True).

CC: @jplab @LaisRast

Component: geometry

Keywords: pyramid, lawrence polytope, combinatorial polyhedron

Author: Jonathan Kliem

Branch/Commit: 7e25b29

Reviewer: Laith Rastanawi

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

@kliem kliem added this to the sage-9.1 milestone Feb 12, 2020
@kliem
Copy link
Contributor Author

kliem commented Feb 12, 2020

New commits:

364a796migrate is_pyramid to combinatorial polyhedron
3af855dmigrate is_lawrence_polytope to combinatorial polyhedron

@kliem
Copy link
Contributor Author

kliem commented Feb 12, 2020

Branch: public/29189

@kliem
Copy link
Contributor Author

kliem commented Feb 12, 2020

Commit: 3af855d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2020

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

db79f1elittle improvement

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2020

Changed commit from 3af855d to db79f1e

@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

Changed branch from public/29189 to public/29189-reb

@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

Changed commit from db79f1e to c47fc68

@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

New commits:

04cd0cfmigrate is_pyramid to combinatorial polyhedron
7cf6c20migrate is_lawrence_polytope to combinatorial polyhedron
da9e835little improvement
c47fc68applied changes from 28608

@LaisRast
Copy link

LaisRast commented Apr 3, 2020

comment:4

Seems good to me.
Is there a reason why you do not put is_pyramid as a cached method?

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

Changed commit from c47fc68 to 77433d7

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

Changed branch from public/29189-reb to public/29189-reb2

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

comment:5

Well, I never considered caching it, because it is trivial. But I guess it doesn't matter.


New commits:

06f9329migrate is_pyramid to combinatorial polyhedron
ebc9e94migrate is_lawrence_polytope to combinatorial polyhedron
4f163bflittle improvement
97db2deapplied changes from 28608
77433d7caching `is_pyramid`

@LaisRast
Copy link

LaisRast commented Apr 3, 2020

Reviewer: Laith Rastanawi

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

comment:7

Sorry, I forgot the trivial cases.

What is the proper output to polytopes.simplex(0).is_pyramid()?

You cannot construct it as pyramid over one of its faces, because the empty face doesn't have a center.

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

comment:8

Currently this returns False. Is this reasonable?

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

comment:9

Then I should add those cases, because currently this just gives a stupid error message.

@LaisRast
Copy link

LaisRast commented Apr 3, 2020

comment:10

Replying to @kliem:

What is the proper output to polytopes.simplex(0).is_pyramid()?

You cannot construct it as pyramid over one of its faces, because the empty face doesn't have a center.

I suggest the output to be True in this case. It does not need to be constructable in sage.

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

comment:11

I agree. I never liked False either. I'll change it.

@kliem

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2020

Changed commit from 77433d7 to 7e25b29

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2020

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

7e25b29fix small dimensional cases

@kliem
Copy link
Contributor Author

kliem commented Apr 3, 2020

New commits:

7e25b29fix small dimensional cases

New commits:

7e25b29fix small dimensional cases

@LaisRast
Copy link

LaisRast commented Apr 3, 2020

comment:15

It looks good now.

@vbraun
Copy link
Member

vbraun commented Apr 9, 2020

Changed branch from public/29189-reb2 to 7e25b29

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

3 participants