Opened 6 years ago
Closed 5 years ago
#22310 closed enhancement (fixed)
Use PPL for facet normals of full-dimensional polytopes
Reported by: | novoselt | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.6 |
Component: | geometry | Keywords: | days85 |
Cc: | vbraun, tscrim | Merged in: | |
Authors: | Andrey Novoseltsev | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | d244793 (Commits, GitHub, GitLab) | Commit: | d24479380a46e868d64c9fe8a228813259ad9414 |
Dependencies: | #22309 | Stopgaps: |
Description (last modified by )
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
Change History (10)
comment:1 Changed 6 years ago by
- Branch set to u/novoselt/PPL_for_fulldim_normals
comment:2 Changed 6 years ago by
- Cc vbraun added
- Commit set to d24479380a46e868d64c9fe8a228813259ad9414
- Status changed from new to needs_review
comment:3 Changed 5 years ago by
- Description modified (diff)
comment:4 Changed 5 years ago by
- Cc tscrim added
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.
comment:5 Changed 5 years ago by
Are the changes in schemes/toric
all coming from different orderings of (some of) the outputs, or are they honest changes?
comment:6 Changed 5 years ago by
- Keywords days85 added
- Reviewers set to Travis Scrimshaw
comment:7 Changed 5 years ago by
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 ;-)
comment:8 Changed 5 years ago by
- Status changed from needs_review to positive_review
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.
comment:9 Changed 5 years ago by
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...
comment:10 Changed 5 years ago by
- Branch changed from u/novoselt/PPL_for_fulldim_normals to d24479380a46e868d64c9fe8a228813259ad9414
- Resolution set to fixed
- Status changed from positive_review to closed
Hey Volker! Looking forward to non-full-dimensional case: many years ago #9188 promised in documentation that
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:
Make dual nef-partitions conveniently ordered
Fix nef-partition ordering in doctests
Add NefCompleteIntersection.cohomology_class
Add Cayley polytopes/cones to dosctring of NefPartition
Add PPL representation to LatticePolytope
Use PPL for computing vertices of LatticePolytope
Fix doctests - mostly due to different order of vertices
Use PPL for facet normals of full-dimensional polytopes
Update a lot of toric doctests for the new facet order