search

sagemath / sage

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

polyhedra level set doctest failure #8709

Closed jhpalmieri closed 14 years ago

jhpalmieri commented 14 years ago

On one platform (an itanium machine running red hat), I see this:

File "/home/palmieri/cleo/sage-4.4.alpha0/devel/sage/sage/geometry/polyhedra.py", line 3147:
    sage: for lset in polytopes.cross_polytope(2).face_lattice().level_sets(): print lset[0]
Expected:
    (None, (0, 1, 2, 3))
    ((1,), (2, 3))
    ((1, 2), (3,))
    ((0, 1, 2, 3), None)
Got:
    (None, (0, 1, 2, 3))
    ((0,), (1, 2))
    ((1, 2), (3,))
    ((0, 1, 2, 3), None)

This is with Sage 4.4.alpha0, and it comes from the patch in ticket #8650.

CC: @sagetrac-mhampton @novoselt

Component: geometry

Author: Marshall Hampton

Reviewer: Andrey Novoseltsev

Merged: sage-4.4.alpha1

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

5d2aaf09-c963-473a-bf79-1f742a72700f commented 14 years ago

apply instead of 8650 patch

novoselt commented 14 years ago
comment:1

Attachment: trac_8709.patch.gz

It looks like some sorting issue. I personally think that it would be nice to have some standard sorting of faces (and it is nice to have this sorting documented).

LatticePolytope's sort faces of dimension 0 according to the generating vertices (so that the i-th 0-dimensional face is generated by the i-th vertex) and codimension 1 faces according to the containing facets (so that the i-th codimension-1 face is contained in the i-th facet). I think that these ways of sorting are very natural and it would be nice to have the same for Polyhedra with its V- and H-representations. For other faces there is probably no "canonical" choice, but it would be nice to have something fixed. (Middle dimensional faces of LatticePolytope's are sorted using one of the above ways, depending on working with an "initial polytope" or its polar - this is done to preserve polar duality of faces for reflexive polytopes.)

5d2aaf09-c963-473a-bf79-1f742a72700f commented 14 years ago
comment:2

The attached patch avoids this sorting issue by only examining the extremes of the face lattice, which is where the problems from 8650 were.

novoselt commented 14 years ago
comment:3

The code in the patch is the same as in #8650, therefore it passes all other doctests just as well as before. The sorting issue is not an issue anymore with the modified doctest for these changes. So positive review, but it still would be nice to add some sorting in the future, especially for dim=0 and codim=1 faces.

novoselt commented 14 years ago

Reviewer: Andrey Novoseltsev

novoselt commented 14 years ago

Author: Marshall Hampton

jhpalmieri commented 14 years ago
comment:4

I'm confused: how is this possibly going to apply over the patch at #8650 (which has been merged into 4.4.alpha0)?

novoselt commented 14 years ago
comment:5

Oops, good point. I am attaching a new patch that just replaces the doctest. Can I still leave it at positive review?..

novoselt commented 14 years ago

Attachment: trac_8709_fix_sorting_issue_from_8650_on_itanium.patch.gz

Apply only this one.

jhpalmieri commented 14 years ago
comment:6

Replying to @novoselt:

Oops, good point. I am attaching a new patch that just replaces the doctest. Can I still leave it at positive review?..

Yes.

jhpalmieri commented 14 years ago

Merged: sage-4.4.alpha1

jhpalmieri commented 14 years ago
comment:7

Merged "trac_8709_fix_sorting_issue_from_8650_on_itanium.patch" into 4.4.alpha1.