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

Polytopes: Initial documentation #468

Merged
merged 10 commits into from
Jun 28, 2021
Merged

Polytopes: Initial documentation #468

merged 10 commits into from
Jun 28, 2021

Conversation

lkastner
Copy link
Member

@lkastner lkastner commented Jun 9, 2021

This pr is a draft of the documentation of the Polytopes part of Oscar.jl.

@lkastner lkastner added the documentation Improvements or additions to documentation label Jun 9, 2021
@lkastner
Copy link
Member Author

lkastner commented Jun 9, 2021

@benlorenz said that the docs will build for a draft pr, right now it is a hassle to build the docs locally.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good to me. Is there a reason to keep this as a draft, instead of simply merging it?

Comment on lines 17 to 18
- Theobald + Joswig
- Ziegler
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you can use bibtex references (see docs/oscar_references.bib; to cite something, use [bibkey](@cite).


Polyhedra are convex bodies. When a polyhedron $P \subset \mathbb{R}^n$ is bounded, it is called a polytope and the fundamental theorem of polytopes states that it may be written as the convex hull of finitely many points in $\mathbb{R}^n$. That is $$P = \textrm{conv}(p_1,\ldots,p_N), p_i \in \mathbb{R}^n.$$ Writing $P$ in this way is called its $V$-representation.

Since many polyhedral computations are done through ```polymake```, the structure ```Polyhedron``` contains a pointer ```pm_polytope``` to the corresponding ```polymake``` object. Thus, all functionality of ```Polymake.jl``` is accessible by calling a ```Polymake.jl``` function on this pointer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use triple backticks here instead of single backticks?


Since many polyhedral computations are done through ```polymake```, the structure ```Polyhedron``` contains a pointer ```pm_polytope``` to the corresponding ```polymake``` object. Thus, all functionality of ```Polymake.jl``` is accessible by calling a ```Polymake.jl``` function on this pointer.

**Warning**: Polyhedra in ```polymake``` and ```Polymake.jl``` use homogeneous coordinates. The polyhedra in ```Oscar``` use affine coordinates.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be a "warning admonition"? See https://juliadocs.github.io/Documenter.jl/stable/showcase/#Warning-admonition

@fingolfin fingolfin requested a review from fieker June 17, 2021 09:48

A subset $P \subseteq \mathbb{R}^n$ is called a (metric) polyhedron if it can be written as the intersection of finitely many closed affine halfspaces in $\mathbb{R}^n$.
That is, there exists a matrix $A$ and a vector $b$ such that
$$P = P(A,b) = \{ x \in \mathbb{R}^n \mid Ax \leq b\}.$$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe its a bot late, but we should think about xA leq b as well - after all, matrices are supposed to be row matrices...

@@ -87,11 +107,34 @@ Base.length(iter::VertexPointIterator) = nvertices(iter.p)
"""
vertices(P::Polyhedron, [,as::Type{T} = Points])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you considered vertices(::Type{bla}, P) as an alternative to (P, as = bla)? Julia odes this for round, floor, ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and we also do this elsewhere in Oscar (e.g. for order of groups, where one specify the desired integer type for the return value). So the caller would write vertices(Points, P). And you could still allow vertices(P) by providing a convenience method to set the default: vertices(P::Polyhedron) = vertices(Points, P)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the first argument is not the type of the return value, so how does this fit with the functions you mention?

Copy link
Contributor

@fieker fieker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favour of merging and then improving further.

@fieker
Copy link
Contributor

fieker commented Jun 17, 2021 via email

@fieker fieker marked this pull request as ready for review June 28, 2021 06:27
@fieker fieker merged commit 872ceb5 into master Jun 28, 2021
@fieker fieker deleted the docs/Polytopes branch June 28, 2021 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants