Opened 2 years ago
Closed 2 years ago
#29189 closed enhancement (fixed)
Migrate `is_lawrence_polytope` and `is_pyramid` to combinatorial polyhedron
Reported by: | gh-kliem | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.1 |
Component: | geometry | Keywords: | pyramid, lawrence polytope, combinatorial polyhedron |
Cc: | jipilab, gh-LaisRast | Merged in: | |
Authors: | Jonathan Kliem | Reviewers: | Laith Rastanawi |
Report Upstream: | N/A | Work issues: | |
Branch: | 7e25b29 (Commits, GitHub, GitLab) | Commit: | 7e25b29ea4cb646f910f3ba3bcb08c1dbee1d2aa |
Dependencies: | Stopgaps: |
Description (last modified by )
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)
.
Change History (16)
comment:1 Changed 2 years ago by
- Branch set to public/29189
- Commit set to 3af855d1c25df37d86578bb1de17ce82123c4f02
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
- Commit changed from 3af855d1c25df37d86578bb1de17ce82123c4f02 to db79f1e37c35761078fb3e4cbce444f4670461f7
Branch pushed to git repo; I updated commit sha1. New commits:
db79f1e | little improvement
|
comment:3 Changed 2 years ago by
- Branch changed from public/29189 to public/29189-reb
- Commit changed from db79f1e37c35761078fb3e4cbce444f4670461f7 to c47fc684125a3a84edcaf78f67c6160064bf41a6
comment:4 Changed 2 years ago by
Seems good to me.
Is there a reason why you do not put is_pyramid
as a cached method?
comment:5 Changed 2 years ago by
- Branch changed from public/29189-reb to public/29189-reb2
- Commit changed from c47fc684125a3a84edcaf78f67c6160064bf41a6 to 77433d7ad52ad07d4fbed331c0fed384998aa86e
Well, I never considered caching it, because it is trivial. But I guess it doesn't matter.
New commits:
06f9329 | migrate is_pyramid to combinatorial polyhedron
|
ebc9e94 | migrate is_lawrence_polytope to combinatorial polyhedron
|
4f163bf | little improvement
|
97db2de | applied changes from 28608
|
77433d7 | caching `is_pyramid`
|
comment:6 Changed 2 years ago by
- Reviewers set to Laith Rastanawi
- Status changed from needs_review to positive_review
comment:7 follow-up: ↓ 10 Changed 2 years ago by
- Status changed from positive_review to needs_work
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.
comment:8 Changed 2 years ago by
Currently this returns False
. Is this reasonable?
comment:9 Changed 2 years ago by
Then I should add those cases, because currently this just gives a stupid error message.
comment:10 in reply to: ↑ 7 Changed 2 years ago by
Replying to gh-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.
comment:11 Changed 2 years ago by
I agree. I never liked False
either. I'll change it.
comment:12 Changed 2 years ago by
- Description modified (diff)
comment:13 Changed 2 years ago by
- Commit changed from 77433d7ad52ad07d4fbed331c0fed384998aa86e to 7e25b29ea4cb646f910f3ba3bcb08c1dbee1d2aa
Branch pushed to git repo; I updated commit sha1. New commits:
7e25b29 | fix small dimensional cases
|
comment:14 Changed 2 years ago by
- Status changed from needs_work to needs_review
comment:15 Changed 2 years ago by
- Status changed from needs_review to positive_review
It looks good now.
comment:16 Changed 2 years ago by
- Branch changed from public/29189-reb2 to 7e25b29ea4cb646f910f3ba3bcb08c1dbee1d2aa
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
migrate is_pyramid to combinatorial polyhedron
migrate is_lawrence_polytope to combinatorial polyhedron