Opened 3 years ago
Last modified 3 years ago
#28646 closed enhancement
Use CombinatorialPolyhedron to obtain faces of polyhedra — at Version 11
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: | #17215 | Stopgaps: |
Description (last modified by )
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
.
There is a conflict with #17215, as it assumes the faces in the old order. However, if this here looks good, #17215 can also be marked dependent on #28646.
Change History (11)
comment:1 Changed 3 years ago by
- Branch set to public/28646
- Commit set to ca5405111f0733513f8a1010b4237d4a4c644280
- Dependencies set to #28625
- Status changed from new to needs_review
comment:2 Changed 3 years ago by
- Commit changed from ca5405111f0733513f8a1010b4237d4a4c644280 to 6828a751122c3f9f779f21a914788b79110dfeda
Branch pushed to git repo; I updated commit sha1. New commits:
6828a75 | avoid import of PolyhedronFace; use existing method
|
comment:3 follow-up: ↓ 5 Changed 3 years ago by
- 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 3 years ago by
- Commit changed from 6828a751122c3f9f779f21a914788b79110dfeda to d85f05534c216adedb07174a5f155e63007c0986
Branch pushed to git repo; I updated commit sha1. New commits:
d85f055 | simplified input/output
|
comment:5 in reply to: ↑ 3 Changed 3 years ago by
Replying to jipilab:
I would vote to change
face_dimension
to simplydimension
.
This is to be consistent with the method faces
`.
comment:6 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:7 Changed 3 years ago by
- Dependencies changed from #28625 to #28625, #17215
There will be a conflict with #17215.
comment:8 Changed 3 years ago by
- 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:
d9cb649 | polyhedron uses CombinatorialPolyhedron for :meth:`faces`; added :meth:`face_generator`
|
comment:9 Changed 3 years ago by
- Commit changed from d9cb64930ee3473eea787cf91136a71d04488163 to 849e09a9e95523b3e742f4c6f2e4fd48f65bdd3d
Branch pushed to git repo; I updated commit sha1. New commits:
849e09a | fixed a new failing test
|
comment:10 Changed 3 years ago by
- Commit changed from 849e09a9e95523b3e742f4c6f2e4fd48f65bdd3d to 4d53c14735f3435fe91d876d336a1af104997015
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a4c3025 | polyhedron uses CombinatorialPolyhedron for :meth:`faces`; added :meth:`face_generator`
|
5f284dd | avoid import of PolyhedronFace; use existing method
|
d15170d | simplified input/output
|
4d53c14 | fixed a new failing test
|
comment:11 Changed 3 years ago by
- Description modified (diff)
I accidentally pulled #28625 instead of #28621. As #28625 is closed, I don't feel like fixing the branch.
New commits:
added combinatorial polyhedron as an attribute for polyhedra
Merge branch 'public/28607' of git://trac.sagemath.org/sage into public/28621
used CombinatorialPolyhedron to compute f_vector
give an error message for polytopes in some cases; removed incorrect example
now we get a precice error message for inexact truncated dodecahedron
subsequent calls for f_vector fail if first attempt fails
Merge branch 'public/28625' of git://trac.sagemath.org/sage into face_iter
polyhedron uses CombinatorialPolyhedron for :meth:`faces`; added :meth:`face_generator`