Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve face generator of polyhedra by exposing FaceIterator class #29654

Closed
kliem opened this issue May 5, 2020 · 22 comments
Closed

Improve face generator of polyhedra by exposing FaceIterator class #29654

kliem opened this issue May 5, 2020 · 22 comments

Comments

@kliem
Copy link
Contributor

kliem commented May 5, 2020

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

Depends on #28894

CC: @jplab @LaisRast @videlec

Component: geometry

Keywords: polyhedra, face iterator

Author: Jonathan Kliem

Branch/Commit: 3ba34fd

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/29654

@kliem kliem added this to the sage-9.1 milestone May 5, 2020
@kliem
Copy link
Contributor Author

kliem commented May 5, 2020

New commits:

b4474b0a face iterator subclass that yield PolyhedronFace

@kliem
Copy link
Contributor Author

kliem commented May 5, 2020

Commit: b4474b0

@kliem
Copy link
Contributor Author

kliem commented May 5, 2020

Branch: public/29654

@kliem kliem modified the milestones: sage-9.1, sage-9.2 May 5, 2020
@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

Changed commit from b4474b0 to 82fe300

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

New commits:

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

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

Changed branch from public/29654 to public/29654-reb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

Changed commit from 82fe300 to 2f274e9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

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

2f274e9coverage and small improvement

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

New commits:

2f274e9coverage and small improvement

New commits:

2f274e9coverage and small improvement

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

Dependencies: #28894

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

comment:5

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

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

Changed branch from public/29654-reb to public/29654-reb2

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

Changed commit from 2f274e9 to d36da4a

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 12, 2020

comment:6

Needs rebase

@kliem
Copy link
Contributor Author

kliem commented Jul 12, 2020

comment:7

Somebody fixed a docstring syntax mistake of mine.

@kliem
Copy link
Contributor Author

kliem commented Jul 12, 2020

comment:8

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

@kliem
Copy link
Contributor Author

kliem commented Jul 12, 2020

Changed commit from d36da4a to 3ba34fd

@kliem
Copy link
Contributor Author

kliem commented Jul 12, 2020

Changed branch from public/29654-reb2 to public/29654-reb3

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2020

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2020

comment:9

This looks like a nice improvement.

@kliem
Copy link
Contributor Author

kliem commented Aug 11, 2020

comment:10

Thank you.

@vbraun
Copy link
Member

vbraun commented Aug 14, 2020

Changed branch from public/29654-reb3 to 3ba34fd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants