Opened 7 years ago
Closed 7 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)
Change History (23)
Changed 7 years ago by novoselt
Changed 7 years ago by novoselt
Changed 7 years ago by novoselt
Changed 7 years ago by novoselt
comment:1 Changed 7 years ago by novoselt
comment:2 Changed 7 years ago by mhampton
- Status changed from new to assigned
comment:3 Changed 7 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.
comment:4 Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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: ↓ 13 Changed 7 years ago by novoselt
Sorry for a probably dumb question, but what is a "rebase?"
comment:13 in reply to: ↑ 12 Changed 7 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
comment:14 Changed 7 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 7 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 7 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 7 years ago by mabshoff
- Resolution set to fixed
- Status changed from new to closed
Merged in Sage 3.0.1.rc0
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/