Opened 8 years ago
Closed 4 years ago
#14116 closed defect (wontfix)
update polymake interface to 2.14rc1
Reported by:  tkluck  Owned by:  mhampton 

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  geometry  Keywords:  
Cc:  was, mkoeppe  Merged in:  
Authors:  Timo Kluck  Reviewers:  KarlDieter Crisman, Frédéric Chapoton, Matthias Köppe 
Report Upstream:  N/A  Work issues:  source tar link, fix doctests, info for how to install, add doctests 
Branch:  public/ticket/14416 (Commits, GitHub, GitLab)  Commit:  50f6da95b155a9739c546b2f0fcd5f6edd09546f 
Dependencies:  #13768, #13767  Stopgaps: 
Description (last modified by )
I've written a new interface for polymake using its shared library interface, while trying to stay backward compatible with what there was before.
I've classified this as a defect, since the current interface doesn't work anymore.
http://www.polymake.org/lib/exe/fetch.php/download/polymake2.14r1minimal.tar.bz2
Attachments (2)
Change History (60)
Changed 8 years ago by
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
Needs to be rebased to #13432.
comment:3 Changed 8 years ago by
Uploading rebased patch next. But:
Compiling sage/geometry/polymake/polymake.pyx because it changed. Cythonizing sage/geometry/polymake/polymake.pyx Error compiling Cython file:  ... def save(self, filename): self.thisptr.save(<string><char*>filename) def __getattr__(self, propertyname): propertyname = propertyname.upper() return <str>get_property_wrapper(self.thisptr[0], <string><char*>propertyname).c_str() ^  sage/geometry/polymake/polymake.pyx:247:92: default encoding required for conversion from 'char const *' to 'str object'
This is presumably related to your comment about getting rid of std::string cpp_string = <string><char*>python_string
stuff; the Cython in Sage is now 5.19.
Error compiling Cython file:  ... p = i.numerator() if callable(i.numerator) else i.numerator q = i.denominator() if callable(i.denominator) else i.denominator entries.push_back(Rational(p,q)) else: entries.push_back(<Rational><int>i) matrix = new Matrix[Rational](rows, cols, entries.begin()) ^  sage/geometry/polymake/polymake.pyx:268:67: Cannot assign type 'iterator' to 'vector[Rational]'
No idea, though the message seems like it would be clear to someone who knew Cython.
Changed 8 years ago by
comment:4 Changed 8 years ago by
kcrisman: I'm considering replacing my own work by what Burcin has done with pypolymake. He works around some cython issues in a much nicer way. Have you looked at that, too?
comment:5 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.10
Have you looked at that, too?
No. It's not exactly clear to me what to do (though this seems likely), and the forums suggest that it doesn't have as much functionality as the interface here, though I may be misinterpreting that. It would be nice to have it in an spkg form, though I understand that is different from the lmonade philosophy.
Also note this polymake user's attempt, though apparently they later just used pypolymake. I assume that user is not yourself?
comment:6 Changed 8 years ago by
In particular, the following things seems like hacks:
ln s /usr/local/sage4.7.1/devel/sage/c_lib/include /usr/local/sage4.7.1/local/include/sage
and this apparently required patch

sageenv
diff git a/sageenv b/sageenv
a b 219 219 mkdir p "$MPLCONFIGDIR" 220 220 fi 221 221 222 LD_PRELOAD="$SAGE_LOCAL/lib/libpolymake.so" 223 export LD_PRELOAD 224 222 225 LD_LIBRARY_PATH="$SAGE_ROOT/local/lib/:$LD_LIBRARY_PATH" && export LD_LIBRARY_PATH 223 226 # The following is needed for openmpi: 224 227 LD_LIBRARY_PATH="$SAGE_ROOT/local/lib/openmpi:$LD_LIBRARY_PATH" && export LD_LIBRARY_PATH
which I'll point out would no longer apply directly, as the LD_LIBRARY_PATH
code has changed, though one could just add the lines, of course. And of course sageenv
doesn't live in local/bin
any more, but in spkg/bin
.
comment:7 Changed 8 years ago by
I just pushed some initial work to
https://github.com/tkluck/sage/tree/polymakeupgrade
It integrates pypolymake with Sage, while keeping backwards compatibility with the old interface. Additionally, it adds an interface allowing you to specify polytopes by Sage expressions.
I'm having some trouble getting the doctests to work, since polymake has a habit of "spamming" stderr. (Actually, it outputs citation information, which is worthwhile. We do need to deal with it, though.) Also, trying to be backwards compatible both with Sage and with pypolymake leads to ugliness such as having both n_points
and num_points
methods.
I'm doing development in the new integrated git repository, which is supposed to replace the mercurial repositories sometime this Summer. Here are instructions for reviewing my branch. They include cloning the repository and building Sage from source, so this takes a lot of time. I will also make mercurial patches available for review at some point.
$ # clone the repository $ git clone git://github.com/tkluck/sage.git $ cd sage $ git checkout t polymakeupgrade $ # get the upstream package for polymake $ cd upstream $ wget http://polymake.org/lib/exe/fetch.php/download/polymake2.12rc3.tar.bz2 $ mv polymake2.12{rc3,}.tar.bz2 $ cd .. $ # build sage $ MAKE="make j4" make $ # install the polymake package $ ./sage i polymake $ # build the polymake interface, now that the package is available $ ./sage b
comment:8 Changed 8 years ago by
How about
 Move the whole stuff to
sage/libs/polymake
 Don't care about backward compatibility, the polymake library interface (like the PPL library interface) is not supposed to be the userfacing part
 Integrate polymake as one of the backends for polyhedra in Sage (and only this will be in sage/geometry)
 the
PolymakeProxy
can go away, the polyhedron backend will raise an error if polymake is not installed.
comment:9 Changed 8 years ago by
Also, why do we need to hardcode the RPATH? This will break during relocation, fyi.
comment:10 Changed 8 years ago by
Thanks for your comments, Volker. I removed hardcoding the RPATH, which seems to have been a leftover from early debugging. I also agree with your idea about eventually making polymake just a backend for the polyhedron class.
In anticipation, I did as you suggested and moved the library interface code to sage/libs/polymake
. I have not made steps to integrate it with the existing polyhedron class and I have also not removed the backward compatibility, including the PolymakeProxy
thing.
I'm not sure if we need to do all this integration work before upgrading polymake and merging this into Sage. Currently, the object polymake
is part of the global namespace on startup, and it is broken. This fixes it.
comment:11 Changed 8 years ago by
Can you actually attach your patch? ;)
There is also a note about Cython 0.17pre that is useless now. Can you remove it while you are at it?
I agree that we don't have to do everything on this ticket. But which base rings does polymake support, and which ones are wrapped here? The polymake web page says something about quadratic field extensions which might be interesting...
comment:12 Changed 8 years ago by
The code is up at github: https://github.com/tkluck/sage/tree/polymakeupgrade
I'll make this into a mercurial patch at some point, but I haven't yet. These kind of updates (simultaneously updating a package and its library interface) are exactly the kind of thing the integrated repository was setup to do.
comment:13 Changed 8 years ago by
 Branch set to u/tkluck/ticket/14116
 Dependencies #13768, #13767 deleted
comment:14 Changed 8 years ago by
 Dependencies set to #13768, #13767
reinstate dependencies after they got lost by using the new devscripts.
comment:15 Changed 7 years ago by
 Commit set to eebd8ec51e4082905710325aeb3c09d019838bb9
 Milestone changed from sage5.10 to sage6.4
Last 10 new commits:
6cbe8ae  Fix wrong doctests

e511e06  Fix failing of doctests due to credits

b3fda3b  some initial work on the GRAPH property

315bd12  Some initial work to make pickling possible

a30b030  move polymake interface to sage.libs.polymake

f049b0b  name of module is sage.libs.polymake.polymake

7ab0ddd  catch some possible c++ exceptions

7c20845  make load work, and thereby pickling

e2ebcf4  coordinates should be a tuple, not a generator

eebd8ec  should have tracked __init__.py file for polymake

comment:16 Changed 7 years ago by
 Branch changed from u/tkluck/ticket/14116 to public/ticket/14416
 Commit changed from eebd8ec51e4082905710325aeb3c09d019838bb9 to e14a9f2e18d3e091ceaf7a922ca5e2ff91effb7a
comment:17 Changed 7 years ago by
Frédéric, have you actually successfully installed polymake and used this, or did you just do community service in updating this to a git branch? Just curious.
comment:18 Changed 7 years ago by
Timo, did you want to update to a newstyle spkg, or was that Frédéric? In which case someone would have to provide a link here to what to put in the upstream/
folder.
comment:19 Changed 7 years ago by
I am trying the new interface, but can't get Sage to build the pyx file.
$ touch src/sage/libs/polymake/polymake.pyx $ ./sage br <stuff not involving that file> sage:
and of course that causes problems with
sage: P = polymake.convex_hull([[1,0,0,0], [1,0,0,1], [1,0,1,0], [1,0,1,1], [1,1,0,0], [1,1,0,1], [1,1,1,0], [1,1,1,1]])
or
sage: from sage.libs.polymake import polymake
which don't work for this reason.
comment:20 Changed 7 years ago by
Ah, I think the problem is that I had to do it from the command line and sage sh
.
sage: sage.misc.package.is_package_installed('polymake') False
Fixing that 'by hand', it WORKS!!!
comment:21 Changed 7 years ago by
 Reviewers set to KarlDieter Crisman, Frédéric Chapoton
 Status changed from new to needs_review
Two concrete things:
 Please make ALL TESTS here optional. Because they pretty much all fail if you don't have polymake, not just the part that imports polymake.
 Exactly one (!) test failure.
********************************************************************** File "src/sage/libs/polymake/polymake.pyx", line 76, in sage.libs.polymake.polymake Failed example: n.graph() # optional  polymake Exception raised: ValueError: invalid value for an input string property
I don't know what this is about, however.
comment:22 Changed 7 years ago by
 Status changed from needs_review to needs_work
 Work issues set to source tar link, fix doctests, info for how to install, add doctests
I think that we need the following for a positive review.
 Fix the doctest issues in the previous comment.
 Provide a source tarball.
 Provide explicit instructions for how to acquire any prereqs for polymake (notably any Perl ones)
 Lots and lots of functions have no doctests. :(
This also definitely should close #5488.
comment:23 Changed 7 years ago by
 Cc was added
Try the source package at http://sage.math.washington.edu/home/kcrisman/polymake2.12.tar.gz  possibly with Mac tar annoying warnings, sorry. If putting this in upstream
works then we are in good shape. I'll note I do get a different set of checksums, so that may have to be fixed. Where did those come from? Someone must have had the src
for that!
By the way  to William  this should enable polymake installation within Sage easy in the cloud (modulo a Perl module or two).
comment:24 Changed 7 years ago by
 Commit changed from e14a9f2e18d3e091ceaf7a922ca5e2ff91effb7a to faa22c6b6e5cd42bd138d8fca06cd8f03d8a601c
comment:25 Changed 7 years ago by
How can I manually install the package to test it ?
Looking at the developer manual, I have copied the tar in my upstream
dir and then tried sage i polymake
, but that fails by trying to upload the sources from the web.
What should I do ?
comment:26 Changed 7 years ago by
I think if you put the tar in the upstream
and then do sage f polymakex.y.z
it should work. the sage i
will try the 'official' (and broken) optional package.
comment:27 Changed 7 years ago by
Thanks, but alas, no, this does not work..
sage i polymake2.12 Found local metadata for polymake2.12 Attempting to download package polymake2.12 ... IOError: [Errno 404] Not Found: '//www.sagemath.org/packages/upstream/polymake/' Error: failed to download package polymake2.12
comment:28 Changed 7 years ago by
Read carefully. Put the tar file I uploaded in comment:23 in $SAGE_ROOT/upstream
, then
sage f polymake2.12
At least, I hope this will work. See also #13768.
comment:29 Changed 7 years ago by
Ooops, sorry for misreading the f
.
Still not working. I have got your polymake2.12.tar.gz in my $SAGE_ROOT/upstream
and nothing among sage (i or f) (polymake or polymake2.12)
works
I have tried both on a clean develop and on the branch here.
comment:30 Changed 7 years ago by
Can you give me the error message when you do sage f polymake2.12
? Maybe something is corrupted in the upload.
comment:31 Changed 7 years ago by
ls lg upstream/po* rwrr 1 users 1565069 oct. 17 2013 upstream/polybori0.8.3.tar.bz2 rwrr 1 users 14504274 nov. 27 13:27 upstream/polymake2.12.tar.gz rwrr 1 users 41376 oct. 17 2013 upstream/polytopes_db20120220.tar.bz2
sage f polymake2.12 Found local metadata for polymake2.12 Attempting to download package polymake2.12 >>> Trying to download http://www.sagemath.org/packages/upstream/polymake/ [Traceback (most recent call last): File "<stdin>", line 35, in <module> File "/homes/doua/chapoton/sage/local/lib/python/urllib.py", line 240, in retrieve fp = self.open(url, data) File "/homes/doua/chapoton/sage/local/lib/python/urllib.py", line 208, in open return getattr(self, name)(url) File "/homes/doua/chapoton/sage/local/lib/python/urllib.py", line 359, in open_http return self.http_error(url, fp, errcode, errmsg, headers) File "/homes/doua/chapoton/sage/local/lib/python/urllib.py", line 376, in http_error return self.http_error_default(url, fp, errcode, errmsg, headers) File "<stdin>", line 17, in http_error_default IOError: [Errno 404] Not Found: '//www.sagemath.org/packages/upstream/polymake/' Error: failed to download package polymake2.12
comment:32 Changed 7 years ago by
Huh. It really shouldn't try to do that. Did you use the branch on this ticket? Otherwise it would assume it's an oldstyle spkg, I believe, in which case this error message would make sense.
comment:33 Changed 7 years ago by
Yes, I am doing that sitting on the branch here, having built this branch.
comment:34 Changed 7 years ago by
Ok, I'll see if I can find some time to try this on sage.math or some other system. I suspect it's due to the packaging etc.
comment:35 Changed 7 years ago by
Okay, I can confirm this problem. Looking into it.
comment:36 Changed 7 years ago by
I think the problem is that now the checksum file includes a line about where the tarball lives  see also here.
For now, try applying

build/pkgs/polymake/checksums.ini
diff git a/build/pkgs/polymake/checksums.ini b/build/pkgs/polymake/checksums.i index e219804..75aba08 100644
a b 1 sha1=a990ea31a68740cbf1aba02d49ec23ef589c173d 2 md5=f07ecafc7a946001c38cdd20e6434b09 3 cksum=1918099647 1 tarball=polymakeVERSION.tar.gz 2 sha1=6488bde98cc4f50f4c327ee55fae1140e2f1c1be 3 md5=b3351c8b772177a198ac0e905f07de99 4 cksum=1659608855
As noted, there are various odd Perl dependencies, see here for some Linux details.
comment:37 Changed 7 years ago by
Thanks. This now starts the installation, which fails later because of some Perl warnings. As I am not root on the machine on which I tried, I will have to try later on my own machine.
comment:38 Changed 7 years ago by
Okay. You should be able to ignore the
WARNING: perl module Term::ReadLine::Gnu required for polymake not found on your machine.
warning, we just won't be able to guarantee using polymake as a commandline option without that (and on Mac that is very tricky, probably much less so on Linux). Something like
sudo perl MCPAN e 'install XML::LibXSLT'
should suffice for the xml stuff, assuming you have the other prereqs they recommend, I guess.
comment:39 Changed 7 years ago by
I get the same build failure on my own machine (after installing all of polymake dependencies) and on the machine of my department, namely:
In file included from /usr/lib/perl/5.18/CORE/perl.h:3482:0, from /home/chapoton/sage/local/var/tmp/sage/build/polymake2.12/src/include/core/polymake/perl/Ext.h:22, from Ext.xs:18: Ext.xs: In function 'pm_perl_get_cx_curpad': /usr/lib/perl/5.18/CORE/av.h:62:26: error: 'PADLIST' has no member named 'sv_u' #define AvARRAY(av) ((av)>sv_u.svu_array) ^ Ext.xs:45:17: note: in expansion of macro 'AvARRAY' return (SV**)AvARRAY(((AV**)AvARRAY(CvPADLIST(cv)))[d+1]); ^ Ext.xs:45:32: note: in expansion of macro 'AvARRAY' return (SV**)AvARRAY(((AV**)AvARRAY(CvPADLIST(cv)))[d+1]); ^ make[2]: *** [Ext.o] Error 1
comment:40 Changed 7 years ago by
This is almost certainly because you have too new (!) of a Perl  see this report. See also e.g. http://forum.polymake.org/viewtopic.php?f=8&p=942#p942 which is really annoying, these things happen a lot.
It seems pretty likely that the 2.13 version should fix this. I will see if that still builds on my 10.7 Mac.
See also http://www.polymake.org/doku.php/workshop1214 which points out that Mac 10.10 compatibility is not yet achieved, probably partly for some related reason. Basically, polymake is definitely still in "optional" territory, if not experimental still!
comment:41 Changed 7 years ago by
Try this instead of the previous, and the package at http://sage.math.washington.edu/home/kcrisman/polymake2.13.tar.gz . I'm currently seeing how/whether this performs right on my machine.

build/pkgs/polymake/checksums.ini
diff git a/build/pkgs/polymake/checksums.ini b/build/pkgs/polymake/checksums.i index e219804..a45c75e 100644
a b 1 sha1=a990ea31a68740cbf1aba02d49ec23ef589c173d 2 md5=f07ecafc7a946001c38cdd20e6434b09 3 cksum=1918099647 1 tarball=polymakeVERSION.tar.gz 2 sha1=72f32a1e0b8a9f8d8e12ef7563c481de1d44cd67 3 md5=92da21355405790c6c816304569a7b12 4 cksum=890996601 
build/pkgs/polymake/packageversion.txt
diff git a/build/pkgs/polymake/packageversion.txt b/build/pkgs/polymake/packa index 3e162f0..ae656d4 100644
a b 1 2.1 21 2.13 
build/pkgs/polymake/spkginstall
diff git a/build/pkgs/polymake/spkginstall b/build/pkgs/polymake/spkginstall index ce6723d..c7a52d1 100755
a b fi 9 9 cd src 10 10 11 11 ./configure prefix="$SAGE_LOCAL" withboost="$SAGE_LOCAL" withgmp="$SAGE 12 withmpfr="$SAGE_LOCAL" withreadline="$SAGE_LOCAL" with fink12 withmpfr="$SAGE_LOCAL" withreadline="$SAGE_LOCAL" withoutf 13 13 if [ $? ne 0 ]; then 14 14 echo "Failed to configure." 15 15 exit 1
comment:42 Changed 7 years ago by
This doesn't work for me, though. Aargh.
comment:43 Changed 7 years ago by
on my department's machine, I got that:
Unrecognized option withreadline=/homes/doua/cha/sage/local; Try ./configure help for detailed information Failed to configure.
and the configure is still beleaving it is in 6.4.rc2
comment:44 Changed 6 years ago by
 Commit changed from faa22c6b6e5cd42bd138d8fca06cd8f03d8a601c to c52e7981f859febc7ca6eca2eff51d36450532bb
Branch pushed to git repo; I updated commit sha1. New commits:
c52e798  Merge branch 'public/ticket/14416' into 6.7

comment:45 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.8
comment:46 Changed 6 years ago by
 Commit changed from c52e7981f859febc7ca6eca2eff51d36450532bb to 50f6da95b155a9739c546b2f0fcd5f6edd09546f
Branch pushed to git repo; I updated commit sha1. New commits:
50f6da9  Merge branch 'public/ticket/14416' into 6.10.b1

comment:47 Changed 6 years ago by
 Description modified (diff)
comment:48 Changed 6 years ago by
 Milestone changed from sage6.8 to sage6.10
 Summary changed from update polymake interface to 2.12rc3 to update polymake interface to 2.14rc1
comment:49 Changed 5 years ago by
 Milestone changed from sage6.10 to sage7.0
comment:50 Changed 5 years ago by
New tentative at #20892 for polymake3.0
comment:51 Changed 5 years ago by
 Cc mkoeppe added
Matthias, are you going to repurpose this and use some of it for the updated interface now that you have things working in #20892 for upgrading polymake?
comment:52 Changed 5 years ago by
I think Vincent may be working on it; and it sounds like it will be on a new ticket rather than this one here.
comment:53 Changed 5 years ago by
I am indeed working on the Cython interface to polymake. I should come out with a minimal version by the end of the week.
comment:54 Changed 4 years ago by
 Milestone changed from sage7.0 to sageduplicate/invalid/wontfix
 Status changed from needs_work to needs_review
I'm proposing to close this outdated ticket because newer efforts have made it obsolete.
comment:55 Changed 4 years ago by
Sounds reasonable  just be sure to point to the tickets which overtake it here :)
comment:56 Changed 4 years ago by
comment:57 Changed 4 years ago by
 Reviewers changed from KarlDieter Crisman, Frédéric Chapoton to KarlDieter Crisman, Frédéric Chapoton, Matthias Köppe
 Status changed from needs_review to positive_review
comment:58 Changed 4 years ago by
 Resolution set to wontfix
 Status changed from positive_review to closed
Closing tickets in the sageduplicate/invalid/wontfix module with positive_review (i.e. someone has confirmed they should be closed).
A related effort is https://bitbucket.org/burcin/pypolymake