Opened 6 years ago

Closed 6 years ago

#2755 closed enhancement (fixed)

[with patch, positive review] lattice_polytope.py update

Reported by: novoselt Owned by: mabshoff
Priority: minor Milestone: sage-3.0.1
Component: geometry Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Finally, the patch with the second version of the module... Does what was proposed on Sage Days 7 and has doctests for all functions (tests for such things as setstate do not have explicit calls but certainly use the required functions).

ADDITIONS:

convex_hull(), minkowski_sum(), NEFPartition.dual()

ReflexivePolytope?(), ReflexivePolytopes?()

LatticePolytopeClass?.plot3d(), LatticePolytopeClass?.skeleton* (points, graph, plot)

Vertices of polytopes are now computed by default.

Little shortcuts like edges() or point().

Examples/tests for each function.

ATTACHED FILES:

Saved Sage objects for sequences of all reflexive polytopes in 2 and 3 dimensions with some precomputed data and dictionaries to these sequences allowing fast identification of the isomorphism class under GL(Z) action.

The module assumes they are located in DB_HOME/reflexive_polytopes/

Attachments (6)

lattice_polytope2.patch (74.4 KB) - added by novoselt 6 years ago.
reflexive_polytopes_2d.sobj (1.6 KB) - added by novoselt 6 years ago.
reflexive_polytopes_2d_dict.sobj (201 bytes) - added by novoselt 6 years ago.
reflexive_polytopes_3d_dict.sobj (73.0 KB) - added by novoselt 6 years ago.
trac_2755_review1.patch (1.2 KB) - added by mhampton 6 years ago.
minor changes; apply after previous patch
lp_patch_for_3.0.1.alpha1.patch (73.7 KB) - added by novoselt 6 years ago.
Updated patch

Download all attachments as: .zip

Change History (23)

Changed 6 years ago by novoselt

Changed 6 years ago by novoselt

Changed 6 years ago by novoselt

Changed 6 years ago by novoselt

comment:1 Changed 6 years ago by novoselt

There is one more file which is too big for attaching it to the ticket. All four are available at
http://sage.math.washington.edu/home/novoselt/patch%202008-04-01/

comment:2 Changed 6 years ago by mhampton

  • Status changed from new to assigned

comment:3 Changed 6 years ago by mhampton

  • Owner changed from mhampton to mabshoff
  • Priority changed from major to minor
  • Status changed from assigned to new
  • Summary changed from [with patch, needs review] lattice_polytope.py update to [with patch, positive review modulo minor changes] lattice_polytope.py update

Nice job! This is a positive review, pending the very minor changes I am attaching as a patch. 100% coverage, passes all tests on my mac intel 10.4.11 machine.

In the future, it might be worth replacing the convex_hull function by my cddlib interface, since it seems that PALP is not configured to handle large polytopes in higher dimensions (for example, it seems extremely fragile starting in dimension 5).

The patch makes convex_hull a little more robust by explicitly casting the input to ZZ; previously it would crash on python int lists.

Changed 6 years ago by mhampton

minor changes; apply after previous patch

comment:4 Changed 6 years ago by novoselt

Sorry for crashes on large dimension - it is my fault, not PALP. I have encountered this issue before but forgot about it since usually polytopes I work with are small enough.

The problem is in piping to poly.x - if the output is too big to fit into the buffer, the output is never read. A fast workaround is to use temp files as it is done in lattice_polytope._palp function, but it maybe slower. On the other hand, I didn't actually benchmark it and interface to a C-library definitely should be better.

comment:5 Changed 6 years ago by mabshoff

Let's make a push to get this merged. What is the status on the requested changes? Or should we merge this as is and then hope that people will finish to polish this code?

Cheers,

Michael

comment:6 Changed 6 years ago by was

I think we should merge this as is. Andrey wrote to me just now and wrote: "I had the impression that the "minor change" was the extra patch with ZZ-conversion and otherwise the current version is adequate."

comment:7 Changed 6 years ago by mabshoff

  • Summary changed from [with patch, positive review modulo minor changes] lattice_polytope.py update to [with patch, positive review] lattice_polytope.py update

Ok, where do we stick the sobj files? I think they should end up in "data_location = DB_HOME + '/reflexive_polytopes/'". This is $SAGE_ROOT/data/reflexive_polytopes/ - so I would need to create that directory. I will play around with this and see what happens.

Thoughts?

Cheers,

Michael

comment:8 Changed 6 years ago by mabshoff

Looking at the problem some more it seems that we need an lattice_polytope-db.spkg. That is the way we deal with all the other databases in the SAGE_DATA directory. This should be fairly trivial to do and not require formal voting due to its minuscule size.

Cheers,

Michael

comment:9 Changed 6 years ago by novoselt

What exactly this package should do? Should it be an optional package? (In this case some functions should probably be rewritten to react in a nice way to the absence of data files.)

comment:10 Changed 6 years ago by mabshoff

Nah, it will be standard and I will take care of it shortly. You can take over from there and maybe add larger optional databases for polytopes of higher dimension if that makes sense.

Cheers,

Michael

comment:11 Changed 6 years ago by mabshoff

  • Summary changed from [with patch, positive review] lattice_polytope.py update to [with patch, positive review, needs rebase] lattice_polytope.py update

Ok, good news and bad news:

Bad news first: tice_polytope2.patch needs a rebase against Sage 3.0.1.alpha1 - out in 10 minutes.

sage-3.0.1.alpha1/devel/sage$ patch -p1 --dry-run < lattice_polytope2.patch
patching file sage/geometry/all.py
Hunk #1 succeeded at 3 with fuzz 1 (offset 2 lines).
patching file sage/geometry/lattice_polytope.py
Hunk #2 FAILED at 57.
1 out of 50 hunks FAILED -- saving rejects to file sage/geometry/lattice_polytope.py.rej

But I create a spkg with the polytope db data with all four files, which has been merged in Sage 3.0.1.alpha1.

Cheers,

Michael

comment:12 follow-up: Changed 6 years ago by novoselt

Sorry for a probably dumb question, but what is a "rebase?"

comment:13 in reply to: ↑ 12 Changed 6 years ago by mabshoff

Replying to novoselt:

Sorry for a probably dumb question, but what is a "rebase?"

Hi,

no dumb questions here, just dumb answers. "rebase" in this context means that you should fix the patch so it does apply cleanly against the latest release. 3.0.1.alpha1 just came out and there is a binary for sage.math in case you don't want to build from scratch yourself. The problem in the patch was in the second hunk and since that was a rather large one I didn't want to fiddle in it and potentially break things.

Let me know if you have any more questions.

Cheers,

Michael

Changed 6 years ago by novoselt

Updated patch

comment:14 Changed 6 years ago by novoselt

  • Summary changed from [with patch, positive review, needs rebase] lattice_polytope.py update to [with patch, positive review, rebase done] lattice_polytope.py update

ZZ conversion patch still should be applied, I forgot about that one.

comment:15 Changed 6 years ago by mabshoff

  • Summary changed from [with patch, positive review, rebase done] lattice_polytope.py update to [with patch, positive review, seemingly unrelated doctest failure] lattice_polytope.py update

The two new patches apply, but now I get the following, probably harmless, doctest failure:

sage -t  devel/sage/sage/rings/number_field/totallyreal_rel.py
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.0.1.rc0/tmp/totallyreal_rel.py", line 98:
    sage: sage.rings.number_field.totallyreal_rel.integral_elements_in_box(K, [[0,5],[0,5]])
Expected:
    [0, 5, 3, -alpha + 2, -alpha + 3, 1, 2, 4, alpha + 2, alpha + 3]
Got:
    [0, 5, -alpha + 2, -alpha + 3, 1, 2, 3, 4, alpha + 2, alpha + 3]
**********************************************************************

I will contact Craig Citro and John Voight to see what their take on this is. Once this is resolved I will merge both patches.

Cheers,

Michael

comment:16 Changed 6 years ago by mabshoff

  • Summary changed from [with patch, positive review, seemingly unrelated doctest failure] lattice_polytope.py update to [with patch, positive review] lattice_polytope.py update

John says:

Yes, the ordering of the elements does not at all affect the
correctness of the output--the most mathematically correct thing would
be to output a set.   This change can be due to any number of things,
but it's probably not worth ascertaining the exact cause.

JV 

Ergo: positive review since I will fix the doctest issue. Let's hope this is not CPU or endianess dependent.

Cheers,

Michael

comment:17 Changed 6 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.0.1.rc0

Note: See TracTickets for help on using tickets.