Opened 2 years ago
Closed 2 years ago
#29654 closed enhancement (fixed)
Improve face generator of polyhedra by exposing `FaceIterator` class
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  geometry  Keywords:  polyhedra, face iterator 
Cc:  jipilab, ghLaisRast, vdelecroix  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Matthias Koeppe 
Report Upstream:  N/A  Work issues:  
Branch:  3ba34fd (Commits, GitHub, GitLab)  Commit:  3ba34fd622d33f8430a09e7c943a40ed1c805e56 
Dependencies:  #28894  Stopgaps: 
Description
Currently, the face generator of polyhedra is a wrapper of the FaceIterator
class, which is intended for combinatorial polyhedra.
However, it takes only little effort to modify this to return a PolyhedronFace
instead of a CombinatorialFace
on next
. The class FaceIterator
has the advantage that it exposes extra features of FaceIterator
. Currently, there is only the possibility of ignoring sub or supfaces, but there might be more features added later.
It also simplifies it a lot for the user to obtain a PolyhedronFace
from FaceIterator
as the needed function is not exactly easy to find.
Follow up:
 Use
FaceIterator
to obtain the meet of (some) vertices or the join of (some) facets.
This is motivated by https://ask.sagemath.org/question/34485/whatisthemostefficientwaytolookupafaceinthefacelatticeofapolyhedron/#50965
Change History (11)
comment:1 Changed 2 years ago by
 Branch set to public/29654
 Commit set to b4474b0c4807dce2362a769ff8072a197e8e9a67
 Milestone changed from sage9.1 to sage9.2
comment:2 Changed 2 years ago by
 Branch changed from public/29654 to public/29654reb
 Commit changed from b4474b0c4807dce2362a769ff8072a197e8e9a67 to 82fe300f3511d7f5cde7cb054084c54ba020f865
comment:3 Changed 2 years ago by
 Commit changed from 82fe300f3511d7f5cde7cb054084c54ba020f865 to 2f274e9a794fce8705e518fc07074346e732a4e2
Branch pushed to git repo; I updated commit sha1. New commits:
2f274e9  coverage and small improvement

comment:4 Changed 2 years ago by
 Status changed from new to needs_review
comment:5 Changed 2 years ago by
 Branch changed from public/29654reb to public/29654reb2
 Commit changed from 2f274e9a794fce8705e518fc07074346e732a4e2 to d36da4a262a2e02686bc0f966a6fb005a10c98f9
 Dependencies set to #28894
There was a merge conflict with #28894, so I rebased.
New commits:
0c44221  important attributes of iterator in structure

efb0bd3  src/simplification of doctests

53fd2a2  fixed failing doctest

a6d2b2d  a face iterator subclass that yield PolyhedronFace

1476675  representation like Polyhedron_base

47fc7cd  account for FaceIterator > FaceIterator_base

295039a  documentation

d36da4a  coverage and small improvement

comment:6 Changed 2 years ago by
Needs rebase
comment:7 Changed 2 years ago by
Somebody fixed a docstring syntax mistake of mine.
comment:8 Changed 2 years ago by
 Branch changed from public/29654reb2 to public/29654reb3
 Commit changed from d36da4a262a2e02686bc0f966a6fb005a10c98f9 to 3ba34fd622d33f8430a09e7c943a40ed1c805e56
I also noticed that I had most of the documentation in the base class, which I think it is bad design, because it is not exposed.
So I moved it to FaceIterator
, because this one is exposed and is the most basic case of FaceIterator_base
.
New commits:
3f92d2d  fixed merge conflict

3ba34fd  move documenation to exposed class

comment:9 Changed 2 years ago by
 Reviewers set to Matthias Koeppe
 Status changed from needs_review to positive_review
This looks like a nice improvement.
comment:10 Changed 2 years ago by
Thank you.
comment:11 Changed 2 years ago by
 Branch changed from public/29654reb3 to 3ba34fd622d33f8430a09e7c943a40ed1c805e56
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
a face iterator subclass that yield PolyhedronFace