Opened 2 years ago
Closed 2 years ago
#29189 closed enhancement (fixed)
Migrate `is_lawrence_polytope` and `is_pyramid` to combinatorial polyhedron
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  geometry  Keywords:  pyramid, lawrence polytope, combinatorial polyhedron 
Cc:  jipilab, ghLaisRast  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/29189reb
 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/29189reb to public/29189reb2
 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 followup: ↓ 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 ghkliem:
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/29189reb2 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