#15240 closed enhancement (fixed)
Switch lattice polytopes to point collections
Reported by:  novoselt  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  geometry  Keywords:  toric sd53 
Cc:  vbraun  Merged in:  
Authors:  Andrey Novoseltsev  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  e5c9506 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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
andFan
);  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 polytopefrommatrix constructor:
sage: LatticePolytope(identity_matrix(3)) /home/novoselt/sage.git/src/bin/sageipython:1: DeprecationWarning: constructing lattice polytopes from matrices is deprecated! See http://trac.sagemath.org/15240 for details. #!/usr/bin/env python 2d lattice polytope in 3d 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.
Change History (32)
comment:1 Changed 9 years ago by
 Branch changed from /u/novoselt/lp2pc to u/novoselt/lp2pc
comment:2 Changed 9 years ago by
 Commit set to dbf840e69f45bfa15cec6ceb774f91dccb5a5d16
comment:3 Changed 9 years ago by
 Commit changed from dbf840e69f45bfa15cec6ceb774f91dccb5a5d16 to 00a1a86f935993a6d3891a570cb16dc53d57cc8f
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. 
comment:4 followup: ↓ 6 Changed 9 years ago by
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: "3d lattice polytope in 4d lattice N", perhaps with "lattice" replaced by "reflexive" automatically and with the index for 2 and 3d 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 builtin 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!
comment:5 Changed 9 years ago by
 Commit changed from 00a1a86f935993a6d3891a570cb16dc53d57cc8f to 9e2512e40f2f3428b63e7dc46170a207bcf36ccc
Branch pushed to git repo; I updated commit sha1. New commits:
[changeset:9e2512e]  Deprecate projective_space polytope. 
comment:6 in reply to: ↑ 4 ; followup: ↓ 7 Changed 9 years ago by
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: "3d lattice polytope in 4d lattice N", perhaps with "lattice" replaced by "reflexive" automatically and with the index for 2 and 3d 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
andoctahedron(2)
better disappear as well, not sure how did I come up with "octa" for vertices other than 8!
Should probably be cross_polytope
;)
comment:7 in reply to: ↑ 6 Changed 9 years ago by
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 3d 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.
comment:8 Changed 9 years ago by
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
comment:9 Changed 9 years ago by
 Milestone changed from sage6.0 to sage6.1
comment:10 Changed 9 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:11 Changed 8 years ago by
 Commit changed from 9e2512e40f2f3428b63e7dc46170a207bcf36ccc to 75e66b5a9e1cfe50b6d7b7f1ea1e7c339fb85d95
Branch pushed to git repo; I updated commit sha1. New commits:
75e66b5  Merge 6.2.beta5 into lp2pc

comment:12 Changed 8 years ago by
 Commit changed from 75e66b5a9e1cfe50b6d7b7f1ea1e7c339fb85d95 to df1722a8fb4166be0bcc5401995094a6a70a4d20
Branch pushed to git repo; I updated commit sha1. New commits:
df1722a  Adjust new calls to deprecated functions and remove <TAB> from merging.

comment:13 Changed 8 years ago by
 Commit changed from df1722a8fb4166be0bcc5401995094a6a70a4d20 to 9658e0c79e2c929a63bfe6621adddb0267c50e87
Branch pushed to git repo; I updated commit sha1. New commits:
9658e0c  Changed _repr_, deprecated desc, simplified error messages, fixed doctests.

comment:14 Changed 8 years ago by
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.
comment:15 Changed 8 years ago by
 Commit changed from 9658e0c79e2c929a63bfe6621adddb0267c50e87 to 31308ea374726a3e98c39164f58dbcb53089ef79
Branch pushed to git repo; I updated commit sha1. New commits:
31308ea  Deprecate normal_form and fix doctests.

comment:16 Changed 8 years ago by
 Commit changed from 31308ea374726a3e98c39164f58dbcb53089ef79 to 2080f3cf0365f4b568ce1129f3d0dc448fe80f11
Branch pushed to git repo; I updated commit sha1. New commits:
2080f3c  Deprecate octahedron.

comment:17 Changed 8 years ago by
 Commit changed from 2080f3cf0365f4b568ce1129f3d0dc448fe80f11 to b06d099d99eb197144c97e5d0c4abd08aee030c0
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.

comment:18 Changed 8 years ago by
 Commit changed from b06d099d99eb197144c97e5d0c4abd08aee030c0 to 9cbcfc4d84c16f28f4b953e1b5e1a53ff5420a80
comment:19 Changed 8 years ago by
 Status changed from new to needs_review
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.
comment:20 Changed 8 years ago by
 Reviewers set to Volker Braun
comment:21 Changed 8 years ago by
Conflicts with the alreadymerged #15990, either resolve now or with the next beta
comment:22 Changed 8 years ago by
 Commit changed from 9cbcfc4d84c16f28f4b953e1b5e1a53ff5420a80 to 46ce7c78478e6ca449a6c3a5f045b598c47946c3
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 raiseplus

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 remotetracking branch 'trac/develop' into ticket/15148

136948a  Merge branch 'u/jpflori/ticket/15148' into raiseplus

46ce7c7  Merge commit '136948a88bbf731a916184b91adf52fb93daa0d4' into lp2pc

comment:23 Changed 8 years ago by
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?
comment:24 Changed 8 years ago by
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?
comment:25 Changed 8 years ago by
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.
comment:26 Changed 8 years ago by
File "/home/buildbot/build/sage/eno1/sage_git/build/local/lib/python2.7/sitepackages/sage/geometry/lattice_polytope.py", line 1058 result = stdout.read() ^ SyntaxError: invalid syntax
comment:27 Changed 8 years ago by
 Commit changed from 46ce7c78478e6ca449a6c3a5f045b598c47946c3 to e5c95061922498ae02ac202e70dda4c82bb8dabe
Branch pushed to git repo; I updated commit sha1. New commits:
e5c9506  I couldn't count parentheses correctly during merge resolution.

comment:28 Changed 8 years ago by
 Branch changed from u/novoselt/lp2pc to e5c95061922498ae02ac202e70dda4c82bb8dabe
 Resolution set to fixed
 Status changed from needs_review to closed
comment:29 Changed 8 years ago by
 Commit e5c95061922498ae02ac202e70dda4c82bb8dabe deleted
Wow, looks like tachyons are real, thanks Volker!
comment:30 Changed 8 years ago by
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
comment:31 Changed 8 years ago by
 Description modified (diff)
Done. Any ideas why do I have
#!/usr/bin/env python
in the end of the warning?..
Branch pushed to git repo; I updated commit sha1. New commits: