Opened 2 years ago

Closed 2 years 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:

Status badges

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 2 years ago by gh-kliem

  • Branch set to public/29654
  • Commit set to b4474b0c4807dce2362a769ff8072a197e8e9a67
  • Milestone changed from sage-9.1 to sage-9.2

New commits:

​b4474b0a face iterator subclass that yield PolyhedronFace

comment:2 Changed 2 years ago by gh-kliem

  • Branch changed from public/29654 to public/29654-reb
  • Commit changed from b4474b0c4807dce2362a769ff8072a197e8e9a67 to 82fe300f3511d7f5cde7cb054084c54ba020f865

New commits:

​b4dca3ea face iterator subclass that yield PolyhedronFace
​f5b7e22representation like Polyhedron_base
​3e5407caccount for FaceIterator -> FaceIterator_base
​82fe300documentation

comment:3 Changed 2 years ago by git

  • Commit changed from 82fe300f3511d7f5cde7cb054084c54ba020f865 to 2f274e9a794fce8705e518fc07074346e732a4e2

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

​2f274e9coverage and small improvement

comment:4 Changed 2 years ago by gh-kliem

  • Status changed from new to needs_review

New commits:

​2f274e9coverage and small improvement

New commits:

​2f274e9coverage and small improvement

comment:5 Changed 2 years ago by gh-kliem

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

​0c44221important attributes of iterator in structure
​efb0bd3src/simplification of doctests
​53fd2a2fixed failing doctest
​a6d2b2da face iterator subclass that yield PolyhedronFace
​1476675representation like Polyhedron_base
​47fc7cdaccount for FaceIterator -> FaceIterator_base
​295039adocumentation
​d36da4acoverage and small improvement

comment:6 Changed 2 years ago by mkoeppe

Needs rebase

comment:7 Changed 2 years ago by gh-kliem

Somebody fixed a docstring syntax mistake of mine.

comment:8 Changed 2 years ago by gh-kliem

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

​3f92d2dfixed merge conflict
​3ba34fdmove documenation to exposed class

comment:9 Changed 2 years ago by mkoeppe

  • 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 gh-kliem

Thank you.

comment:11 Changed 2 years ago by vbraun

  • Branch changed from public/29654-reb3 to 3ba34fd622d33f8430a09e7c943a40ed1c805e56
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.