Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#15240 closed enhancement (fixed)

Switch lattice polytopes to point collections

Reported by: novoselt Owned by:
Priority: major Milestone: sage-6.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:

Status badges

Description (last modified by nthiery)

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.

Change History (32)

comment:1 Changed 9 years ago by novoselt

  • Branch changed from /u/novoselt/lp2pc to u/novoselt/lp2pc

comment:2 Changed 9 years ago by git

  • Commit set to dbf840e69f45bfa15cec6ceb774f91dccb5a5d16

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

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

comment:3 Changed 9 years ago by git

  • 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 follow-up: Changed 9 years ago by 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!

comment:5 Changed 9 years ago by git

  • 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 ; follow-up: Changed 9 years ago by 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 ;-)

comment:7 in reply to: ↑ 6 Changed 9 years ago by 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.

comment:8 Changed 9 years ago by 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

comment:9 Changed 9 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:10 Changed 9 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:11 Changed 8 years ago by git

  • Commit changed from 9e2512e40f2f3428b63e7dc46170a207bcf36ccc to 75e66b5a9e1cfe50b6d7b7f1ea1e7c339fb85d95

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

75e66b5Merge 6.2.beta5 into lp2pc

comment:12 Changed 8 years ago by git

  • Commit changed from 75e66b5a9e1cfe50b6d7b7f1ea1e7c339fb85d95 to df1722a8fb4166be0bcc5401995094a6a70a4d20

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

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

comment:13 Changed 8 years ago by git

  • Commit changed from df1722a8fb4166be0bcc5401995094a6a70a4d20 to 9658e0c79e2c929a63bfe6621adddb0267c50e87

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

9658e0cChanged _repr_, deprecated desc, simplified error messages, fixed doctests.

comment:14 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by git

  • Commit changed from 9658e0c79e2c929a63bfe6621adddb0267c50e87 to 31308ea374726a3e98c39164f58dbcb53089ef79

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

31308eaDeprecate normal_form and fix doctests.

comment:16 Changed 8 years ago by git

  • Commit changed from 31308ea374726a3e98c39164f58dbcb53089ef79 to 2080f3cf0365f4b568ce1129f3d0dc448fe80f11

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

2080f3cDeprecate octahedron.

comment:17 Changed 8 years ago by git

  • Commit changed from 2080f3cf0365f4b568ce1129f3d0dc448fe80f11 to b06d099d99eb197144c97e5d0c4abd08aee030c0

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

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

comment:18 Changed 8 years ago by git

  • Commit changed from b06d099d99eb197144c97e5d0c4abd08aee030c0 to 9cbcfc4d84c16f28f4b953e1b5e1a53ff5420a80

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

0e4ac31Fixed octahedron(2) calls sage/schemes/toric
9cbcfc4Fix the last broken doctests.

comment:19 Changed 8 years ago by novoselt

  • 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 vbraun

  • Reviewers set to Volker Braun

comment:21 Changed 8 years ago by vbraun

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

comment:22 Changed 8 years ago by git

  • Commit changed from 9cbcfc4d84c16f28f4b953e1b5e1a53ff5420a80 to 46ce7c78478e6ca449a6c3a5f045b598c47946c3

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

eb09160trac #11726 implement floordiv for laurent polys in one variable
e27a420trac #11726 one failing doctest corrected
ba866efMerge branch 'public/ticket/11726' into raise-plus
86261b1Improve count_points
55cdcf3Refactor point counting code for hyperelliptic curves.
fc7a569First bunch of fixes and missing examples.
63bf717Rebase on top of #15108.
8029bc6Merge remote-tracking branch 'trac/develop' into ticket/15148
136948aMerge branch 'u/jpflori/ticket/15148' into raise-plus
46ce7c7Merge commit '136948a88bbf731a916184b91adf52fb93daa0d4' into lp2pc

comment:23 Changed 8 years ago by 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?

comment:24 Changed 8 years ago by 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?

comment:25 Changed 8 years ago by 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.

comment:26 Changed 8 years ago by 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

comment:27 Changed 8 years ago by git

  • Commit changed from 46ce7c78478e6ca449a6c3a5f045b598c47946c3 to e5c95061922498ae02ac202e70dda4c82bb8dabe

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

e5c9506I couldn't count parentheses correctly during merge resolution.

comment:28 Changed 8 years ago by vbraun

  • 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 novoselt

  • Commit e5c95061922498ae02ac202e70dda4c82bb8dabe deleted

Wow, looks like tachyons are real, thanks Volker!

comment:30 Changed 8 years ago by 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

comment:31 Changed 8 years ago by novoselt

  • Description modified (diff)

Done. Any ideas why do I have

  #!/usr/bin/env python

in the end of the warning?..

comment:32 Changed 8 years ago by nthiery

  • Description modified (diff)

Thanks!

No clue about the warning.

Note: See TracTickets for help on using tickets.