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

Use PPL for facet normals of full-dimensional polytopes #22310

Closed
novoselt opened this issue Feb 6, 2017 · 12 comments
Closed

Use PPL for facet normals of full-dimensional polytopes #22310

novoselt opened this issue Feb 6, 2017 · 12 comments

Comments

@novoselt
Copy link
Member

novoselt commented Feb 6, 2017

Before:

sage: timeit("LatticePolytope(lattice_polytope.cross_polytope(3).vertices()).facet_normals()")
5 loops, best of 3: 43.2 ms per loop

After:

sage: timeit("LatticePolytope(lattice_polytope.cross_polytope(3).vertices()).facet_normals()")
125 loops, best of 3: 6.81 ms per loop

PPL will of course work for non-full-dimensional polytopes as well, however the treatment of this case is spread around several places and its removal will be treated separately. Once this is done the speed up will be even more significant.

Next in the chain of lattice polytope improvements is #22391

Depends on #22309

CC: @vbraun @tscrim

Component: geometry

Keywords: days85

Author: Andrey Novoseltsev

Branch/Commit: d244793

Reviewer: Travis Scrimshaw

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

@novoselt novoselt added this to the sage-7.6 milestone Feb 6, 2017
@novoselt
Copy link
Member Author

novoselt commented Feb 6, 2017

Branch: u/novoselt/PPL_for_fulldim_normals

@novoselt
Copy link
Member Author

novoselt commented Feb 6, 2017

comment:2

Hey Volker! Looking forward to non-full-dimensional case: many years ago #9188 promised in documentation that

        If this polytope is not full-dimensional, facet normals will be
        orthogonal to the integer kernel of the affine subspace spanned by
        this polytope.

I can't recall any reason why this orthogonality would be of any importance, can you? Trying to ensure it (unless PPL for some reason guarantees it already) involves a somewhat long chain of matrix manipulations that you eventually got correct and while it is sad to remove it, doing so will definitely simplify the code and likely make it noticeably faster.


New commits:

a262ec3Make dual nef-partitions conveniently ordered
6b5972cFix nef-partition ordering in doctests
2e26768Add NefCompleteIntersection.cohomology_class
7afca73Add Cayley polytopes/cones to dosctring of NefPartition
e78ff49Add PPL representation to LatticePolytope
5c286e4Use PPL for computing vertices of LatticePolytope
db3b54eFix doctests - mostly due to different order of vertices
5281cf4Use PPL for facet normals of full-dimensional polytopes
d244793Update a lot of toric doctests for the new facet order

@novoselt
Copy link
Member Author

novoselt commented Feb 6, 2017

Commit: d244793

@novoselt

This comment has been minimized.

@novoselt
Copy link
Member Author

comment:4

Hi, Travis! Since you were so kind to review the dependency, do you care to take a look at this one? The real change is in the first short commit (the second one is doctest fixing) and that one is mostly copy-pasting old code and constructing the same thing from PPL functions rather than PALP format.

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2017

comment:5

Are the changes in schemes/toric all coming from different orderings of (some of) the outputs, or are they honest changes?

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2017

Changed keywords from none to days85

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2017

Reviewer: Travis Scrimshaw

@novoselt
Copy link
Member Author

comment:7

Um, what are "honest changes"? The cause for all of them is the change in ordering. Since toric computations often rely on both vertices and normals, things got affected quite a bit. Some tests had to be adjusted themselves (i.e. not just output) because they relied on indices of normals in the list, e.g. for charts.

I've done my best to go through all affected cases carefully and reading the context to make sure that they still made sense, although I don't know of an easy way to verify my claim ;-)

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2017

comment:8

I would consider than an honest change, but if you say they are equivalent answers, then that is good enough for me. Thanks; positive review.

@novoselt
Copy link
Member Author

comment:9

Thanks a lot! Naturally, feel free to move to the next one, but it gets more involved, although I tried to make each commit sensible on its own...

@vbraun
Copy link
Member

vbraun commented Mar 27, 2017

Changed branch from u/novoselt/PPL_for_fulldim_normals to d244793

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