Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#9062 closed enhancement (fixed)

Add support for toric lattices

Reported by: novoselt Owned by: mhampton
Priority: major Milestone: sage-4.5.2
Component: geometry Keywords:
Cc: vbraun 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 novoselt)

Toric lattices are ZZn'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)

trac_9062_add_support_toric_lattices.patch (27.3 KB) - added by novoselt 9 years ago.
Fixed version.
trac_9062_add_support_for_toric_lattices.patch (27.2 KB) - added by novoselt 9 years ago.
Apply this patch only.
trac_9062-cmp_fix.patch (715 bytes) - added by davidloeffler 9 years ago.
Apply this patch over trac_9062_add_support_for_toric_lattices.patch
trac_9062-cmp_fix.2.patch (1.1 KB) - added by vbraun 9 years ago.
Updated patch

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by novoselt

  • Status changed from new to needs_work

comment:2 Changed 9 years ago by vbraun

  • Cc vbraun added

Looks good, I'll be happy to review the final version!

Changed 9 years ago by novoselt

Fixed version.

Changed 9 years ago by novoselt

Apply this patch only.

comment:3 Changed 9 years ago by novoselt

  • Description modified (diff)
  • Status changed from needs_work to 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 9 years ago by novoselt

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 9 years ago by vbraun

  • Status changed from needs_review to 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 9 years ago by vbraun

  • Reviewers set to vbraun

comment:7 Changed 9 years ago by novoselt

  • Reviewers changed from vbraun to 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 9 years ago by davidloeffler

  • Status changed from positive_review to 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 9 years ago by davidloeffler

Apply this patch over trac_9062_add_support_for_toric_lattices.patch

comment:9 Changed 9 years ago by davidloeffler

  • Status changed from needs_work to needs_review

Here's a tiny patch which makes the fix I suggested.

comment:10 Changed 9 years ago by vbraun

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".

Changed 9 years ago by vbraun

Updated patch

comment:11 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to 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 9 years ago by mpatel

  • Merged in set to sage-4.5.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 9 years ago by mpatel

One or more of #8986, #8987, and #9062 may cause the doctest failures listed at #9590.

Note: See TracTickets for help on using tickets.