Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for toric lattices #9062

Closed
novoselt opened this issue May 27, 2010 · 18 comments
Closed

Add support for toric lattices #9062

novoselt opened this issue May 27, 2010 · 18 comments

Comments

@novoselt
Copy link
Member

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

CC: @vbraun

Component: geometry

Author: Andrey Novoseltsev

Reviewer: Volker Braun

Merged: sage-4.5.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/9062

@vbraun
Copy link
Member

vbraun commented May 27, 2010

comment:2

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

@novoselt
Copy link
Member Author

Attachment: trac_9062_add_support_toric_lattices.patch.gz

Fixed version.

@novoselt
Copy link
Member Author

Attachment: trac_9062_add_support_for_toric_lattices.patch.gz

Apply this patch only.

@novoselt

This comment has been minimized.

@novoselt
Copy link
Member Author

comment:3

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

@novoselt
Copy link
Member Author

comment:4

Functions is_ToricLattice and __hash__ for elements will be added to these new modules in the coming patch for cones at #8986.

@vbraun
Copy link
Member

vbraun commented Jun 3, 2010

comment:5

Very nice. I like it how the M/N lattice elements derive from Vector_integer_dense. Positive review. Applies to Sage 4.4.2.

@vbraun
Copy link
Member

vbraun commented Jun 3, 2010

Reviewer: vbraun

@novoselt
Copy link
Member Author

novoselt commented Jun 3, 2010

Changed reviewer from vbraun to Volker Braun

@novoselt
Copy link
Member Author

novoselt commented Jun 3, 2010

comment:7

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

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 1, 2010

comment:8

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

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 1, 2010

Apply this patch over trac_9062_add_support_for_toric_lattices.patch

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 1, 2010

comment:9

Attachment: trac_9062-cmp_fix.patch.gz

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

@loefflerd loefflerd mannequin added s: needs review and removed s: needs work labels Jul 1, 2010
@vbraun
Copy link
Member

vbraun commented Jul 1, 2010

comment:10

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

@vbraun
Copy link
Member

vbraun commented Jul 1, 2010

Updated patch

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jul 1, 2010

comment:11

Attachment: trac_9062-cmp_fix.2.patch.gz

Looks good to me. Apply trac_9062_add_support_for_toric_lattices.patch and trac_9062-cmp_fix.2.patch.

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

Merged: sage-4.5.2.alpha0

@qed777 qed777 mannequin removed the s: positive review label Jul 20, 2010
@qed777 qed777 mannequin closed this as completed Jul 20, 2010
@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 24, 2010

comment:13

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants