Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#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:

Status badges

Description (last modified by Andrey Novoseltsev)

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 Andrey Novoseltsev 13 years ago.
Fixed version.
trac_9062_add_support_for_toric_lattices.patch (27.2 KB) - added by Andrey Novoseltsev 13 years ago.
Apply this patch only.
trac_9062-cmp_fix.patch (715 bytes) - added by David Loeffler 12 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 Volker Braun 12 years ago.
Updated patch

Download all attachments as: .zip

Change History (17)

comment:1 Changed 13 years ago by Andrey Novoseltsev

Status: newneeds_work

comment:2 Changed 13 years ago by Volker Braun

Cc: Volker Braun added

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

Changed 13 years ago by Andrey Novoseltsev

Fixed version.

Changed 13 years ago by Andrey Novoseltsev

Apply this patch only.

comment:3 Changed 13 years ago by Andrey Novoseltsev

Description: modified (diff)
Status: needs_workneeds_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 Andrey Novoseltsev

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 Volker Braun

Status: needs_reviewpositive_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 Volker Braun

Reviewers: vbraun

comment:7 Changed 13 years ago by Andrey Novoseltsev

Reviewers: vbraunVolker 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 David Loeffler

Status: positive_reviewneeds_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 David Loeffler

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 David Loeffler

Status: needs_workneeds_review

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

comment:10 Changed 12 years ago by Volker Braun

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 12 years ago by Volker Braun

Attachment: trac_9062-cmp_fix.2.patch added

Updated patch

comment:11 Changed 12 years ago by David Loeffler

Status: needs_reviewpositive_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 Mitesh Patel

Merged in: sage-4.5.2.alpha0
Resolution: fixed
Status: positive_reviewclosed

comment:13 Changed 12 years ago by Mitesh Patel

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

Note: See TracTickets for help on using tickets.