Unifying and streamlining Polytopes interfaces

[lkastner]

This is a list of some issues with the current implementation that need to be straightened out. In particular, the Cone and Polyhedron interfaces need to be unified. Some of these issues might already be solved, @alexej-jordan @benlorenz feel free to check off those (or add issues I missed).

• There should be facets(C::Cone) similar to facets(P::Polyhedron)
• facets_as_point_matrix should be integrated into facets using the as parameter
• facets_as_halfspace_matrix_pair should be integrated into facets using the as parameter
• There should be f_vector(C::Cone)
• Should PolyhedronRayIterator, PolyhedralFanRayIterator, and ConeRayIterator be the same?
• Since vertices, rays, and facets return iterators, lineality_space should also return an iterator
• Did we add a constructor for a cone from inequalities yet?
• We should probably add linear_hull(C::Cone) and affine_hull(P::Polyhedron)
• There should be f_vector(F::PolyhedralFan)
• Should we make a submodule for the Polytopes part?
• We need to check that the following n* functions are available wherever applicable: nrays, nfacets, nvertices
• The as parameter does not need separate structs, it should just take e.g. Matrix when returning a matrix, Polyhedron when returning an iterator over polyhedra, etc.

[alexej-jordan]

I checked off some of theses which are already covered within #578 . Functionality is already implemented, I am mostly working on documentation and tests right now. these are points number 1, 2, 3, 5 and 6.
I have implemented a different solution for number 2 and 3, where this is replaced by point_matrix(iter::VectorIterator) so that you now can for example call point_matrix(vertices(P)) (analogue for halfspace_matrix_pair).

Concerning the last point, I do also agree. I would like to apply these changes within #578 because I already interfered with this issue there, and it has a lot to do with the iterators.

I am not sure if we should make a submodule. How is the consensus on the split up into submodules within Oscar?

EDIT: Also, I noticed that facets(as::Type{T}, P::Polyhedron) allows as = Polyhedron. It would be consequent to transfer this to cones, as well. The way data is stored within a HalfspaceIterator (return type of facets for both Polyhedron and Cone) requires point number 7, a way to construct a cone from inequalities, which again should also allow fitting iterator input. In conclusion, these changes should be part of #578 , too.

[fieker]

On Fri, Sep 10, 2021 at 05:22:55PM -0700, alexej-jordan wrote: I checked off some of theses which are already covered within #578 . Functionality is already implemented, I am mostly working on documentation and tests right now. these are points number 1, 2, 3, 5 and 6. I have implemented a different solution for number 2 and 3, where this is replaced by `point_matrix(iter::VectorIterator)` so that you now can for example call `point_matrix(vertices(P))` (analogue for `halfspace_matrix_pair`). Concerning the last point, I do also agree. I would like to apply these changes within #578 because I already interfered with this issue there, and it has a lot to do with the iterators. I am not sure if we should make a submodule. How is the consensus on the split up into submodules within Oscar?

There is no consensus about this at all. We've used submodules here and there to "hide" functions more easily, other than that, we haven't been able to see any serious advantage nor disadvantage in submodules. What is you intend for using them?

…

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #677 (comment)

[lkastner]

So one of the reasons was the struct Point which also exists in the experimental section, but not in the form that we want to use it. If we made a submodule we would not have to come up with an awkward name.

Another reason I was thinking about this is the tabcompletion. If you do Oscar. + tab it will get cluttered after a while, but we could at least group the functions in a meaningful way using modules.

[fieker]

On Wed, Sep 15, 2021 at 07:57:51AM -0700, Lars Kastner wrote: So one of the reasons was the struct `Point` which also exists in the experimental section, but not in the form that we want to use it. If we made a submodule we would not have to come up with an awkward name.

This we need to sort as I think there should be only one Point type if possible.

Another reason I was thinking about this is the tabcompletion. If you do `Oscar.` + tab it will get cluttered after a while, but we could at least group the functions in a meaningful way using modules.

agreed - but has to be balanced with guessing module names. Shouldn't actually be too bad as the same name will have many many implementations, so way fewer names as in Singular/ Gap/ (polymake)

…

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #677 (comment)

[lkastner]

I postpone the submodule question to a different PR.