Opened 2 years ago

Last modified 2 years ago

#28646 closed enhancement

Use CombinatorialPolyhedron to obtain faces of polyhedra — at Version 13

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords: polytopes, combinatorial polyhedron
Cc: jipilab, gh-LaisRast Merged in:
Authors: Jonathan Kliem Reviewers: Jean-Philippe Labbé
Report Upstream: N/A Work issues:
Branch: public/28646-reb (Commits, GitHub, GitLab) Commit: 4d53c14735f3435fe91d876d336a1af104997015
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

We use CombinatorialPolyhedron to obtain the faces of fixed dimensions of a polyhedron.

We add a method face_generator, which iterates over all faces (possibly of fixed dimension).

With this ticket we can obtain the faces much faster and without generating the entire face lattice. The iterator is a true iterator in the sense that it has almost constant memory usage and does and no point store a list of all faces.

This ticket changes the order of the output of faces.

Change History (13)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/28646
  • Commit set to ca5405111f0733513f8a1010b4237d4a4c644280
  • Dependencies set to #28625
  • Status changed from new to needs_review

I accidentally pulled #28625 instead of #28621. As #28625 is closed, I don't feel like fixing the branch.


New commits:

b89610eadded combinatorial polyhedron as an attribute for polyhedra
dfbe2adMerge branch 'public/28607' of git://trac.sagemath.org/sage into public/28621
ed5518bused CombinatorialPolyhedron to compute f_vector
9bdd005give an error message for polytopes in some cases; removed incorrect example
acd671dnow we get a precice error message for inexact truncated dodecahedron
bf85a62subsequent calls for f_vector fail if first attempt fails
36ac386Merge branch 'public/28625' of git://trac.sagemath.org/sage into face_iter
ca54051polyhedron uses CombinatorialPolyhedron for :meth:`faces`; added :meth:`face_generator`

comment:2 Changed 2 years ago by git

  • Commit changed from ca5405111f0733513f8a1010b4237d4a4c644280 to 6828a751122c3f9f779f21a914788b79110dfeda

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

6828a75avoid import of PolyhedronFace; use existing method

comment:3 follow-up: Changed 2 years ago by jipilab

  • Reviewers set to Jean-Philippe Labbé
  • Status changed from needs_review to needs_work

I would vote to change face_dimension to simply dimension.

The INPUT of combinatorial_face_to_polyhedral_face is too verbose to me.

A Polyhedron, the polyhedron containing the face A CombinatorialFace

OUTPUT:

A PolyhedronFace

is sufficient. If you insist to have the links on the documentation ok... But I would not be too verbose here.

comment:4 Changed 2 years ago by git

  • Commit changed from 6828a751122c3f9f779f21a914788b79110dfeda to d85f05534c216adedb07174a5f155e63007c0986

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

d85f055simplified input/output

comment:5 in reply to: ↑ 3 Changed 2 years ago by gh-kliem

Replying to jipilab:

I would vote to change face_dimension to simply dimension.

This is to be consistent with the method faces`.

comment:6 Changed 2 years ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:7 Changed 2 years ago by gh-kliem

  • Dependencies changed from #28625 to #28625, #17215

There will be a conflict with #17215.

comment:8 Changed 2 years ago by gh-kliem

  • Branch changed from public/28646 to public/28646-reb
  • Commit changed from d85f05534c216adedb07174a5f155e63007c0986 to d9cb64930ee3473eea787cf91136a71d04488163
  • Dependencies changed from #28625, #17215 to #17215

Rebased


New commits:

d9cb649polyhedron uses CombinatorialPolyhedron for :meth:`faces`; added :meth:`face_generator`

comment:9 Changed 2 years ago by git

  • Commit changed from d9cb64930ee3473eea787cf91136a71d04488163 to 849e09a9e95523b3e742f4c6f2e4fd48f65bdd3d

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

849e09afixed a new failing test

comment:10 Changed 2 years ago by git

  • Commit changed from 849e09a9e95523b3e742f4c6f2e4fd48f65bdd3d to 4d53c14735f3435fe91d876d336a1af104997015

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a4c3025polyhedron uses CombinatorialPolyhedron for :meth:`faces`; added :meth:`face_generator`
5f284ddavoid import of PolyhedronFace; use existing method
d15170dsimplified input/output
4d53c14fixed a new failing test

comment:11 Changed 2 years ago by gh-kliem

  • Description modified (diff)

comment:12 Changed 2 years ago by gh-bbryant33

  • Status changed from needs_review to positive_review

The tests all pass on my end. face_generator runs for every type of polytope it should run on, and combinatorial_face_to_polyhedral_face runs as it should. The change in order experienced by the various face functions only affected the lists, not the correctness.

comment:13 Changed 2 years ago by gh-kliem

  • Dependencies #17215 deleted
  • Description modified (diff)

Removing the dependency.

#17215 will need small changes in the doctests, once it's done.

Note: See TracTickets for help on using tickets.