Opened 3 years ago

Closed 2 years ago

#22391 closed enhancement (fixed)

Always use PPL for facet normals of lattice polytopes

Reported by: novoselt Owned by:
Priority: major Milestone: sage-8.1
Component: geometry Keywords: sd91, sd90
Cc: Merged in:
Authors: Andrey Novoseltsev Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: baf0a4a (Commits) Commit: baf0a4ad48cffc0a166b6db9ca9ba471f59c93eb
Dependencies: #22310 Stopgaps:

Description (last modified by novoselt)

A follow up to #22310. As promised there, this switch makes even full-dimensional polytopes faster, on the same example we now get

sage: timeit("LatticePolytope(lattice_polytope.cross_polytope(3).vertices()).facet_normals()")
625 loops, best of 3: 976 µs per loop

The reason is that dimension computation used to be quite complicated.

Next in the chain of lattice polytope improvements is #22524

Change History (17)

comment:1 Changed 3 years ago by novoselt

  • Branch set to u/novoselt/PPL_for_normals

comment:2 Changed 3 years ago by novoselt

  • Commit set to b4410f11f98d3bb09d4a2018187b6ff840ea7c87
  • Status changed from new to needs_review

Last 10 new commits:

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
e70f34dMerge tag '7.6.beta3' into PPL_for_normals
b27e343Always use PPL for LatticePolytope.facet_normals()
8e77861Update docstrings for facet normals and constants
ba7656aUse PPL for LatticePolytope.dim()
38bbdb2Don't use embedding in LatticePolytope.distances()
f4a696bImplement point containment test for LatticePolytope
b4410f1Do not rely on distances for containment check

comment:3 Changed 3 years ago by novoselt

  • Description modified (diff)

comment:4 Changed 2 years ago by ursula

I successfully installed this patch and tried some variants on the example. Speed seems to work as advertised:

sage: sim = ReflexivePolytope(3,0)
sage: timeit("sim.facet_normals()")
625 loops, best of 3: 576 ns per loop
Last edited 2 years ago by ursula (previous) (diff)

comment:5 follow-up: Changed 2 years ago by vdelecroix

Have you tried using normaliz? In dimension >= 10 it is likely to be faster. For that reason, it is good to have an optional keyword method= or algorithm= that simplifies speed comparisons.

comment:6 in reply to: ↑ 5 Changed 2 years ago by novoselt

Replying to vdelecroix:

Have you tried using normaliz? In dimension >= 10 it is likely to be faster. For that reason, it is good to have an optional keyword method= or algorithm= that simplifies speed comparisons.

The interest here is mostly in polytopes of dimensions 3-4-5, when the bottleneck is often the interface between programs. Also, the old code was quite convoluted for many reasons and adding different algorithms would be tricky. One of the goals of this and related tickets was to clean up the mess to indeed allow using different backends for certain operations.

comment:7 Changed 2 years ago by roed

  • Keywords sd91 added

comment:8 Changed 2 years ago by ursula

  • Keywords sd90 added

comment:9 Changed 2 years ago by ursula

I tried building again, and now this patch fails, with errors involving sage-ursula/src/build/cythonized/sage/. My guess is that the transition from Sage 7.6 to the current development branch is too great. Do you mind updating the patch?

comment:10 Changed 2 years ago by ursula

  • Status changed from needs_review to needs_work

comment:11 Changed 2 years ago by novoselt

Can you please be more specific in how it fails? The branch merges cleanly and passes tests according to the patchbot.

comment:12 Changed 2 years ago by tscrim

It also works for me. I am guessing what happened is @ursula just used the branch, effectively downgrading her version of Sage to that of the branch, and ran sage -b, which does not rebuild the necessary parts of Sage. In general, I always make sure to merge it ontop of (my version of) develop to avoid regressing Sage (which will include long compilation time). If that is not what [@ursula] did, then try running make build and then sage -t.

comment:13 Changed 2 years ago by tscrim

  • Branch changed from u/novoselt/PPL_for_normals to public/polytopes/PPL_for_normals-22391
  • Commit changed from b4410f11f98d3bb09d4a2018187b6ff840ea7c87 to baf0a4ad48cffc0a166b6db9ca9ba471f59c93eb
  • Milestone changed from sage-7.6 to sage-8.1
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_work to needs_review

I made some small formatting changes to more match our style guide. If my changes look good, then positive review.


New commits:

49be993Merge branch 'u/novoselt/PPL_for_normals' of git://trac.sagemath.org/sage into u/novoselt/PPL_for_normals
baf0a4aA little bit of reviewer cleanup.

comment:14 Changed 2 years ago by novoselt

  • Status changed from needs_review to positive_review

Thank you!!!

Of course your changes look good - anything to get it in ;-) A small question however:

         TypeError: the point M(1, 1) and
-        2-d cone in 2-d lattice N have incompatible lattices!
+         2-d cone in 2-d lattice N have incompatible lattices

Your new line apart from dropping the exclamation point due to exception formatting change also adds a space in front of the second part of the broken line. Is this also the style recommended somewhere? I can't recall ever reading it.

comment:15 Changed 2 years ago by tscrim

Not really, but I feel its gives better grouping that it is part of the error message and not some new line of output, similar to indenting function blocks.

comment:16 Changed 2 years ago by tscrim

Also, Python does not use sentence ending punctuation for any exceptions (or initial uppercase letters).

comment:17 Changed 2 years ago by vbraun

  • Branch changed from public/polytopes/PPL_for_normals-22391 to baf0a4ad48cffc0a166b6db9ca9ba471f59c93eb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.