search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 480 forks source link

Add support for toric lattices #9062

Closed novoselt closed 14 years ago

novoselt commented 14 years ago

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 commented 14 years ago
comment:2

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

novoselt commented 14 years ago

Attachment: trac_9062_add_support_toric_lattices.patch.gz

Fixed version.

novoselt commented 14 years ago

Attachment: trac_9062_add_support_for_toric_lattices.patch.gz

Apply this patch only.

novoselt commented 14 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,29 @@
 Toric lattices are ZZ<sup>n</sup>'s with distinction of their roles (in the simplest case - standard dual lattices M and N).

-Once this patch is finished, it will be the first part of the toric varieties framework #8986-#8989, but so far I made it actually on top of those modules. Creation of cones and fans seems to work as expected. More work is needed on matrix multiplication. Working on it!
+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
novoselt commented 14 years ago
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 commented 14 years ago
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 commented 14 years ago
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 commented 14 years ago

Reviewer: vbraun

novoselt commented 14 years ago

Changed reviewer from vbraun to Volker Braun

novoselt commented 14 years ago
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.)

11d1fc49-71a1-44e1-869f-76be013245a0 commented 14 years ago
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

11d1fc49-71a1-44e1-869f-76be013245a0 commented 14 years ago

Apply this patch over trac_9062_add_support_for_toric_lattices.patch

11d1fc49-71a1-44e1-869f-76be013245a0 commented 14 years ago
comment:9

Attachment: trac_9062-cmp_fix.patch.gz

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

vbraun commented 14 years ago
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 commented 14 years ago

Updated patch

11d1fc49-71a1-44e1-869f-76be013245a0 commented 14 years ago
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.

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago

Merged: sage-4.5.2.alpha0

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:13

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