Opened 21 months ago

Closed 19 months 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:

Status badges

Description (last modified by gh-kliem)

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 21 months ago by gh-kliem

  • Branch set to public/29189
  • Commit set to 3af855d1c25df37d86578bb1de17ce82123c4f02
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 20 months ago by git

  • Commit changed from 3af855d1c25df37d86578bb1de17ce82123c4f02 to db79f1e37c35761078fb3e4cbce444f4670461f7

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

​db79f1elittle improvement

comment:3 Changed 20 months ago by gh-kliem

  • Branch changed from public/29189 to public/29189-reb
  • Commit changed from db79f1e37c35761078fb3e4cbce444f4670461f7 to c47fc684125a3a84edcaf78f67c6160064bf41a6

New commits:

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

comment:4 Changed 19 months ago by gh-LaisRast

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

Last edited 19 months ago by gh-LaisRast (previous) (diff)

comment:5 Changed 19 months ago by gh-kliem

  • 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:

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

comment:6 Changed 19 months ago by gh-LaisRast

  • Reviewers set to Laith Rastanawi
  • Status changed from needs_review to positive_review

comment:7 follow-up: Changed 19 months ago by gh-kliem

  • 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 19 months ago by gh-kliem

Currently this returns False. Is this reasonable?

comment:9 Changed 19 months ago by gh-kliem

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

comment:10 in reply to: ↑ 7 Changed 19 months ago by gh-LaisRast

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 19 months ago by gh-kliem

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

comment:12 Changed 19 months ago by gh-kliem

  • Description modified (diff)

comment:13 Changed 19 months ago by git

  • Commit changed from 77433d7ad52ad07d4fbed331c0fed384998aa86e to 7e25b29ea4cb646f910f3ba3bcb08c1dbee1d2aa

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

​7e25b29fix small dimensional cases

comment:14 Changed 19 months ago by gh-kliem

  • Status changed from needs_work to needs_review

New commits:

​7e25b29fix small dimensional cases

New commits:

​7e25b29fix small dimensional cases

comment:15 Changed 19 months ago by gh-LaisRast

  • Status changed from needs_review to positive_review

It looks good now.

comment:16 Changed 19 months ago by vbraun

  • Branch changed from public/29189-reb2 to 7e25b29ea4cb646f910f3ba3bcb08c1dbee1d2aa
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.