Switch lattice polytopes to point collections

[novoselt]

The goals of this ticket are:

• make the code of LatticePolytope less interdependent for easier maintenance and backend switch;
• use PointCollection container internally to unify it with other classes for toric geometry (Cone and Fan);
• deprecate current methods returning points as matrices so that their output can be changed in the future.

One of the easiest to discover change is the deprecation of the polytope-from-matrix constructor:

sage: LatticePolytope(identity_matrix(3))
/home/novoselt/sage.git/src/bin/sage-ipython:1: DeprecationWarning: constructing lattice polytopes from matrices is deprecated!
See http://trac.sagemath.org/15240 for details.
  #!/usr/bin/env python
2-d lattice polytope in 3-d lattice M

To get the same result without the deprecation warning one has to do

sage: LatticePolytope(identity_matrix(3).columns())

In most cases users had to pack points into a column matrix and then call the lattice polytope constructor. Now they can just omit the packing step.

Note that matrices will eventually be accepted again as valid input since they are "iterables of iterables", but rows of a matrix will be viewed as points, not columns.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:dbf840e]	Deprecated vertices().
[changeset:4f7a367]	Turn off points precomputing for reflexive polytopes.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:00a1a86]	Deprecate constructing lattice polytopes from matrices as well.
[changeset:61ab3a1]	Deprecated points() and fixed doctests.
[changeset:06348e4]	Made doctests pass again.

[novoselt]

Hey Volker - I think it would be nice to deprecate in this ticket everything that we don't want to keep except for the way how faces are handled (which I'd rather do in a follow up ticket). So far I've deprecated constructing polytopes from matrices, perhaps constructing them from files should be deprecated as well and handled by separate functions?

What about "desc"? I'd like to print polytopes similar to cones/fans: "3-d lattice polytope in 4-d lattice N", perhaps with "lattice" replaced by "reflexive" automatically and with the index for 2- and 3-d cases. Should then "desc" be gone and those who really want can use Sage standard "rename"?

Also, I am not sure what the output of "normal_form" should be - keeping it as a matrix does not seem a good choice. Maybe the corresponding polytope in ToricLattice(dim).dual()? Then polytopes from built-in lists will return themselves and be equal to their normal form.

projective_space and octahedron(2) better disappear as well, not sure how did I come up with "octa" for vertices other than 8!

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

[changeset:9e2512e]

Deprecate projective_space polytope.

[vbraun]

Replying to novoselt:

Hey Volker - I think it would be nice to deprecate in this ticket everything that we don't want to keep except for the way how faces are handled (which I'd rather do in a follow up ticket).

Sounds good!

What about "desc"? I'd like to print polytopes similar to cones/fans: "3-d lattice polytope in 4-d lattice N", perhaps with "lattice" replaced by "reflexive" automatically and with the index for 2- and 3-d cases. Should then "desc" be gone and those who really want can use Sage standard "rename"?

+1

Also, I am not sure what the output of "normal_form" should be - keeping it as a matrix does not seem a good choice.

A matrix isn't totally strange, after all the normal form is a version of hermite normal form together with permutation of vertices.

If you want to return it as another LatticePolytope then the constructor should keep the vertices in the specified order. The ambient lattice should be the same as the lattice of the original polytope, imho.

projective_space and octahedron(2) better disappear as well, not sure how did I come up with "octa" for vertices other than 8!

Should probably be cross_polytope ;-)

[novoselt]

Replying to vbraun:

Replying to novoselt:

Also, I am not sure what the output of "normal_form" should be - keeping it as a matrix does not seem a good choice.

A matrix isn't totally strange, after all the normal form is a version of hermite normal form together with permutation of vertices.

If you want to return it as another LatticePolytope then the constructor should keep the vertices in the specified order. The ambient lattice should be the same as the lattice of the original polytope, imho.

I always want to preserve order, especially when the input consists of vertices rather than spanning points ;-) The issue with the same lattice as the original one is that normal forms of equivalent polytopes in different lattices will compare as different, which seems a bit counterintuitive with respect to index, i.e. with my default lattice choice we would have for any 2- or 3-d reflexive polytope

sage: p.normal_form() == ReflexivePolytope(p.dim(), p.index())
True

On the other hand starting with a polytope in N and getting as its normal form something in M isn't great either... So maybe a matrix is the way to go. Perhaps with vertices as rows rather than columns?

Speaking of index - are you OK with the name or something like db_index would be better? I definitely find it very useful, but the meaning of index for morphisms and cones is completely different and there are "more mathematical" indices associated to polytopes, like Gorenstein one.

[vbraun]

Having normal forms compare different if they live in different lattices would be fine with me, for the record.

As for the index, how about .label() or .id()? It could return a pair of (dim, index) that would be valid input for ReflexivePolytope

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​75e66b5

Merge 6.2.beta5 into lp2pc

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​df1722a

Adjust new calls to deprecated functions and remove <TAB> from merging.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​9658e0c

Changed _repr_, deprecated desc, simplified error messages, fixed doctests.

[novoselt]

Picking up the previous discussion, I think using PointCollection for normal_form is a good choice with the lattice being the same as the lattice of the polytope. Then for polytopes in normal form p.vertices() == p.normal_form() without any extra remarks. The index for 2 and 3 dimensions will mean matching these normal forms but ignoring lattices.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​31308ea

Deprecate normal_form and fix doctests.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​2080f3c

Deprecate octahedron.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​c495aa1	Merge branch 6.2.beta7 into lp2pc to take advantage of improved deprecation.
​6988767	sed -i "s/lattice_polytope.octahedron(3)/lattice_polytope.cross_polytope(3)/" `grep -rl "octahedron(3)" src/sage`
​b06d099	Fixed octahedron(n) calls for n!=3 in sage/geometry.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​0e4ac31	Fixed octahedron(2) calls sage/schemes/toric
​9cbcfc4	Fix the last broken doctests.

[novoselt]

OK, I better stop given that a few thousands lines are affected already. Except for lattice_polytope.py they are mostly doctest fixes due to deprecated octahedron and changed representation.

The main thing that is left is fix the mess with polytope faces, but that definitely should be a separate ticket.

[vbraun]

Conflicts with the already-merged #15990, either resolve now or with the next beta

[git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

​eb09160	trac #11726 implement floordiv for laurent polys in one variable
​e27a420	trac #11726 one failing doctest corrected
​ba866ef	Merge branch 'public/ticket/11726' into raise-plus
​86261b1	Improve count_points
​55cdcf3	Refactor point counting code for hyperelliptic curves.
​fc7a569	First bunch of fixes and missing examples.
​63bf717	Rebase on top of #15108.
​8029bc6	Merge remote-tracking branch 'trac/develop' into ticket/15148
​136948a	Merge branch 'u/jpflori/ticket/15148' into raise-plus
​46ce7c7	Merge commit '136948a88bbf731a916184b91adf52fb93daa0d4' into lp2pc

[novoselt]

OK, I've done

git merge 136948a88bbf731a916184b91adf52fb93daa0d4

which is the commit listed as "branch" on #15990 and then resolved the conflict. Was it the way to go?

[novoselt]

When I click on commits on trac, then my two merges with betas are shown as single commits, while this resent change shows also other stuff - what is the difference between merging "develop" and other commits into my branch?

[vbraun]

The betas that you merged are (by definition) already released, so in the trac view they (that is, the second parent of the merge) is excluded. But the ticket that you merged in is not yet released in a beta, so its entire history is shown as well. When I release the next beta containing #15990 then it will be removed from the trac view.

[vbraun]

  File "/home/buildbot/build/sage/eno-1/sage_git/build/local/lib/python2.7/site-packages/sage/geometry/lattice_polytope.py", line 1058
    result = stdout.read()
         ^
SyntaxError: invalid syntax

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​e5c9506

I couldn't count parentheses correctly during merge resolution.

[novoselt]

Wow, looks like tachyons are real, thanks Volker!

[nthiery]

Hi!

Do you mind editing the ticket description so that users can easily find what's the current alternative to constructing lattice polytopes from a matrix?

Thanks!

Nicolas

[novoselt]

Done. Any ideas why do I have

  #!/usr/bin/env python

in the end of the warning?..

[nthiery]

Thanks!

No clue about the warning.