Opened 10 months ago
Closed 7 months ago
#29654 closed enhancement (fixed)
Improve face generator of polyhedra by exposing `FaceIterator` class
Reported by: | gh-kliem | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.2 |
Component: | geometry | Keywords: | polyhedra, face iterator |
Cc: | jipilab, gh-LaisRast, 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/what-is-the-most-efficient-way-to-look-up-a-face-in-the-face-lattice-of-a-polyhedron/#50965
Change History (11)
comment:1 Changed 10 months ago by
- Branch set to public/29654
- Commit set to b4474b0c4807dce2362a769ff8072a197e8e9a67
- Milestone changed from sage-9.1 to sage-9.2
comment:2 Changed 10 months ago by
- Branch changed from public/29654 to public/29654-reb
- Commit changed from b4474b0c4807dce2362a769ff8072a197e8e9a67 to 82fe300f3511d7f5cde7cb054084c54ba020f865
comment:3 Changed 10 months 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 10 months ago by
- Status changed from new to needs_review
comment:5 Changed 10 months ago by
- Branch changed from public/29654-reb to public/29654-reb2
- 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 8 months ago by
Needs rebase
comment:7 Changed 8 months ago by
Somebody fixed a docstring syntax mistake of mine.
comment:8 Changed 8 months ago by
- Branch changed from public/29654-reb2 to public/29654-reb3
- 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 7 months ago by
- Reviewers set to Matthias Koeppe
- Status changed from needs_review to positive_review
This looks like a nice improvement.
comment:10 Changed 7 months ago by
Thank you.
comment:11 Changed 7 months ago by
- Branch changed from public/29654-reb3 to 3ba34fd622d33f8430a09e7c943a40ed1c805e56
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
a face iterator subclass that yield PolyhedronFace