#9062 closed enhancement (fixed)
Add support for toric lattices
Reported by: | Andrey Novoseltsev | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-4.5.2 |
Component: | geometry | Keywords: | |
Cc: | Volker Braun | Merged in: | sage-4.5.2.alpha0 |
Authors: | Andrey Novoseltsev | Reviewers: | Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Toric lattices are ZZ^{n}'s with distinction of their roles (in the simplest case - standard dual lattices M and N).
This patch is a part of the following series adding support for cones/fans and toric varieties to Sage:
Prerequisites:
#8675 - Remove AmbientSpace._constructor
and fix consequences
#8682 - Improve AlgebraicScheme_subscheme.__init__
and AmbientSpace._validate
#8694 - Improve schemes printing and LaTeXing
#8934 - Trivial bug in computing faces of non-full-dimensional lattice polytopes
#8936 - Expose facet inequalities for lattice polytopes
#8941 - _latex_
and origin
for lattice polytopes
Main patches adding new modules:
#9062 - Add support for toric lattices
#8986 - Add support for convex rational polyhedral cones
#8987 - Add support for rational polyhedral fans
#8988 - Add support for toric varieties
#8989 - Add support for Fano toric varieties
Attachments (4)
Change History (17)
comment:1 Changed 13 years ago by
Status: | new → needs_work |
---|
comment:2 Changed 13 years ago by
Cc: | Volker Braun added |
---|
Changed 13 years ago by
Attachment: | trac_9062_add_support_toric_lattices.patch added |
---|
Fixed version.
Changed 13 years ago by
Attachment: | trac_9062_add_support_for_toric_lattices.patch added |
---|
Apply this patch only.
comment:3 Changed 13 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
It will probably work without other "prerequisites," but I tested it with them applied since all got positive review already and hopefully will be merged soon...
comment:4 Changed 13 years ago by
Functions is_ToricLattice
and __hash__
for elements will be added to these new modules in the coming patch for cones at #8986.
comment:5 Changed 13 years ago by
Status: | needs_review → positive_review |
---|
Very nice. I like it how the M/N lattice elements derive from Vector_integer_dense. Positive review. Applies to Sage 4.4.2.
comment:6 Changed 13 years ago by
Reviewers: | → vbraun |
---|
comment:7 Changed 13 years ago by
Reviewers: | vbraun → Volker Braun |
---|
Thank you! (I think authors and reviewers should be listed with their full names rather than trac accounts, this simplifies the job of release managers.)
comment:8 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
While testing I found a heisenbug caused by this patch. If you run "make ptestlong", there is a failure in toric_lattice_element.pyx; but it works fine if you doctest just that file.
The problem is this comparison function:
def __cmp__(self, right): r""" [...] sage: cmp(n, 1) -1 """ c = cmp(type(self), type(right)) if c: return c
The doctest is sensitively dependent on the exact memory locations of different classes, because cmp(type(self), type(right))
compares on memory addresses. I suggest changing the doctest to
sage: n == 1 False
which is much more robust.
David
Changed 12 years ago by
Attachment: | trac_9062-cmp_fix.patch added |
---|
Apply this patch over trac_9062_add_support_for_toric_lattices.patch
comment:9 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
Here's a tiny patch which makes the fix I suggested.
comment:10 Changed 12 years ago by
The same potential issue is in toric_lattice.py
. Here is an updated patch that fixes this one, too. I think this is fine now, if you agree please set to "positive review".
comment:11 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
Looks good to me. Apply trac_9062_add_support_for_toric_lattices.patch
and trac_9062-cmp_fix.2.patch
.
comment:12 Changed 12 years ago by
Merged in: | → sage-4.5.2.alpha0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Looks good, I'll be happy to review the final version!