update polymake interface to 2.14-rc1

[tkluck]

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/polymake-2.14r1-minimal.tar.bz2

Depends on #13768
Depends on #13767

CC: @williamstein @mkoeppe

Component: geometry

Work Issues: source tar link, fix doctests, info for how to install, add doctests

Author: Timo Kluck

Branch/Commit: public/ticket/14416 @ 50f6da9

Reviewer: Karl-Dieter Crisman, Frédéric Chapoton, Matthias Köppe

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

[tkluck]

comment:1

Attachment: trac_14116_polymake_upgrade.patch.gz

A related effort is https://bitbucket.org/burcin/pypolymake

[kcrisman]

comment:2

Needs to be rebased to #13432.

[kcrisman]

comment:3

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.

[kcrisman]

Attachment: trac_14116-polymake-upgrade-rebased.patch.gz

[tkluck]

comment:4

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?

[kcrisman]

comment:5

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?

[kcrisman]

comment:6

In particular, the following things seems like hacks:

ln -s /usr/local/sage-4.7.1/devel/sage/c_lib/include /usr/local/sage-4.7.1/local/include/sage

and this apparently required patch

diff --git a/sage-env b/sage-env
--- a/sage-env
+++ b/sage-env
@@ -219,6 +219,9 @@
     mkdir -p "$MPLCONFIGDIR"
 fi
 
+LD_PRELOAD="$SAGE_LOCAL/lib/libpolymake.so"
+export LD_PRELOAD
+
 LD_LIBRARY_PATH="$SAGE_ROOT/local/lib/:$LD_LIBRARY_PATH" && export LD_LIBRARY_PATH
 # The following is needed for openmpi:
 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 sage-env doesn't live in local/bin any more, but in spkg/bin.

[tkluck]

comment:7

I just pushed some initial work to

https://github.com/tkluck/sage/tree/polymake-upgrade

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 polymake-upgrade
$ # get the upstream package for polymake
$ cd upstream
$ wget http://polymake.org/lib/exe/fetch.php/download/polymake-2.12-rc3.tar.bz2
$ mv polymake-2.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

[vbraun]

comment:8

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 user-facing 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.

[vbraun]

comment:9

Also, why do we need to hardcode the RPATH? This will break during relocation, fyi.

[tkluck]

comment:10

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.

[vbraun]

comment:11

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...

[tkluck]

comment:12

The code is up at github:
https://github.com/tkluck/sage/tree/polymake-upgrade

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.

[tkluck]

Branch: u/tkluck/ticket/14116

[tkluck]

Changed dependencies from #13768, #13767 to none

[tkluck]

Dependencies: #13768, #13767

[tkluck]

comment:14

reinstate dependencies after they got lost by using the new dev-scripts.

[fchapoton]

Commit: eebd8ec

[fchapoton]

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

[fchapoton]

Changed branch from u/tkluck/ticket/14116 to public/ticket/14416

[fchapoton]

Changed commit from eebd8ec to e14a9f2

[sagetrac-git]

Changed commit from faa22c6 to c52e798

[sagetrac-git]

Changed commit from c52e798 to 50f6da9

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

50f6da9

Merge branch 'public/ticket/14416' into 6.10.b1

[videlec]

comment:50

New tentative at #20892 for polymake-3.0

[kcrisman]

comment:51

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?

[mkoeppe]

comment:52

I think Vincent may be working on it; and it sounds like it will be on a new ticket rather than this one here.

[videlec]

comment:53

I am indeed working on the Cython interface to polymake. I should come out with a minimal version by the end of the week.

[mkoeppe]

comment:54

I'm proposing to close this outdated ticket because newer efforts have made it obsolete.

[kcrisman]

comment:55

Sounds reasonable - just be sure to point to the tickets which overtake it here :)

[mkoeppe]

comment:56

That's:

• Create a Polymake pexpect interface #22452: Create a Polymake pexpect interface
• Support interface coercion polymake(X) for Polyhedron #22658: Support interface coercion polymake(X) for Polyhedron
• backend_polymake for Polyhedron #22683: backend_polymake for Polyhedron
  and, in a separate effort,
• package pypolymake #21170: package pypolymake

[kcrisman]

Changed reviewer from Karl-Dieter Crisman, Frédéric Chapoton to Karl-Dieter Crisman, Frédéric Chapoton, Matthias Köppe

[embray]

comment:58

Closing tickets in the sage-duplicate/invalid/wontfix module with positive_review (i.e. someone has confirmed they should be closed).