Replacement of sage/libs/ppl.pyx by pplpy

[vinklein]

The Sage wrappers around ppl are now superseded by the standalone pplpy (Python/Cython compatible). This ticket proposes to replace src/sage/libs/ppl.pyx extension with pplpy. For this, we

• create a new standard package pplpy
• upgrade gmpy2 from optional to standard
• replace the Sage code calling sage/libs/ppl.pyx to call pplpy
• make appropriate deprecation for sage.libs.ppl

Tarball for pplpy: pplpy-0.8.4.tar.gz

Upstream: Reported upstream. Developers acknowledge bug.

CC: @videlec @kiwifb @embray @slel

Component: packages: standard

Keywords: thursdaysbdx, ppl, pplpy, upgrade

Author: Vincent Klein

Branch: 829317e

Reviewer: Dima Pasechnik, François Bissey, Vincent Delecroix, Jeroen Demeyer, Erik Bray

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

[vinklein]

Changed dependencies from #229 to #22928

[vinklein]

Branch: u/vklein/replacement_of_sage_libs_ppl_pyx_by_pplpy

[sagetrac-git]

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

f8bf274

Updating checksum for pplpy pkg

[sagetrac-git]

Commit: f8bf274

[vinklein]

comment:7

For testing purpose you can use the following tarball of pplpy (outdated, use the one in the ticket's description)

[videlec]

comment:8

Salut Vincent,

Your branch appears in red which means that there are merge issues (possibly trivial). Would be better to rebase it on the latest beta.

[videlec]

comment:9

1. Pickling should be fixed

-        sage: TestSuite(Asso).run()
+        sage: TestSuite(Asso).run(skip='_test_pickling')

It works with pplpy used from Python

>>> x = ppl.Variable(0)
>>> y = ppl.Variable(1)
>>> z = ppl.Variable(2)
>>> gs.insert(ppl.point(x+y+2*z))
>>> gs.insert(ppl.point(x-y))
>>> gs.insert(ppl.point(z))
>>> P = ppl.C_Polyhedron(gs)
>>> import pickle
>>> pickle.loads(pickle.dumps(P)) == P
True

2. I don't see the point of the function mpz_to_sage_integer. You can just do [Integer(x) for x in my_mpz_list]. For example

v_coords = (1,) + tuple(mpz_to_sage_integer(v.coefficients()))

can be replaced by

v_coords = (1,) + tuple(Integer(c) for c in v.coefficients())

Moreover, the name is confusing since the fuction converts a list of mpz integers.

3. Some possible simplifications

• In ppl_backend.pyx:
  Integer(g.coefficient(Variable(variable))) / Integer(g.divisor()) ->
  Rational(g.coefficient(Variable(variable)) / g.divisor())

• In ppl_lattice_polytope.py: assert Integer(v.divisor()).is_one() -> assert v.divisor() == 1

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

f19f81e	Integer and rational constructors support gmpy2 params
cd8ef5d	remove OptionalExtension sage.libs.gmpy2
8ced5d3	remove get_gmpy2_path()
674fe62	Add few doctests
2ed4f95	Use of MPZ_Check and MPQ_Check functions from gmpy2 C-API
b4f0c26	Add pplpy as a standard package
257c330	Replace sage.libs.ppl with pplpy package
636051f	Remove sage.libs.ppl extension
f52b3d4	Modify associahedron doctest
dffa134	Updating checksum for pplpy pkg

[sagetrac-git]

Changed commit from f8bf274 to dffa134

[vinklein]

comment:11

Replying to @videlec:

3. Some possible simplifications

• In ppl_backend.pyx:
  Integer(g.coefficient(Variable(variable))) / Integer(g.divisor()) ->
  Rational(g.coefficient(Variable(variable)) / g.divisor())

• In ppl_lattice_polytope.py: assert Integer(v.divisor()).is_one() -> assert v.divisor() == 1

It will be nice, but coercion for gmpy2 types is required for doing this (#23052).

[sagetrac-git]

Changed commit from dffa134 to b3563eb

[sagetrac-git]

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

b3563eb

remove mpz_to_sage_integer function

[videlec]

comment:13

Replying to @vinklein:

Replying to @videlec:

3. Some possible simplifications

• In ppl_backend.pyx:
  Integer(g.coefficient(Variable(variable))) / Integer(g.divisor()) ->
  Rational(g.coefficient(Variable(variable)) / g.divisor())

• In ppl_lattice_polytope.py: assert Integer(v.divisor()).is_one() -> assert v.divisor() == 1

It will be nice, but coercion for gmpy2 types is required for doing this (#23052).

Where? The first involves a direct call Rational(mpq_type) and the second comparison between mpz and Python integer. Which operation is mixing gmpy2 types with Sage types?

[vinklein]

comment:14

Replying to @videlec:

Where? The first involves a direct call Rational(mpq_type) and the second comparison between mpz and Python integer. Which operation is mixing gmpy2 types with Sage types?

You're right, tests failures in ppl_backend.pyx with the suggested code are not coercion related. Forget my previous comment.

[sagetrac-git]

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

3785bf4	assert simplification ppl_lattice_polytope.py
d3875e7	remove skip=('_test_pickling') option for pplpy polyhedron

[sagetrac-git]

Changed commit from b3563eb to d3875e7

[vinklein]

comment:16

As pplpy has evolved to fix pickling with cPickle download pplpy again to update your local package.

[sagetrac-git]

Changed commit from d3875e7 to 7ae21f5

[sagetrac-git]

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

700f72c	remove sage/libs/ppl from doctree
7ae21f5	trac 23024 Bullet list indentation correction

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

08c7a72	Add path of gmpy2 in include_dirs
359a0ff	update checksum and package version
0d4fd2e	Integer and rational constructors support gmpy2 params
7c66a7c	remove OptionalExtension sage.libs.gmpy2
d143dde	remove get_gmpy2_path()
2c56a3f	Add few doctests
e5264fd	Use of MPZ_Check and MPQ_Check functions from gmpy2 C-API
504ab68	Change way of creating a Rational from a mpz
072a15c	add __mpz__() to Rational
858c2e5	trac 23024 update pplpy checksum

[sagetrac-git]

Changed commit from 7ae21f5 to 858c2e5

[vinklein]

comment:19

As pplpy has evolved to fix pickling with cPickle download ​pplpy again to update your local package.

[vinklein]

Changed dependencies from #22928 to #22928, #23309

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

1c1b971	Replace sage.libs.ppl with pplpy package
2a1798e	Remove sage.libs.ppl extension
d1b0b2f	Modify associahedron doctest
db21e1d	Updating checksum for pplpy pkg
eb215c3	remove mpz_to_sage_integer function
53f30f3	assert simplification ppl_lattice_polytope.py
f9a6415	remove skip=('_test_pickling') option for pplpy polyhedron
924984c	remove sage/libs/ppl from doctree
ce863ff	update checksum, snapshot version update in setup.py
7a5812a	Add path of gmpy2 in include_dirs

[vinklein]

comment:204

Ok thank you for your help.

[sagetrac-git]

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

76cfa5f	Trac #23024: upgrade pplpy to 0.8.4
829317e	Trac #23024: add a spkg-postrm script to clean...

[sagetrac-git]

Changed commit from b25b719 to 829317e

[vinklein]

comment:206

Upgrade to pplpy 0.8.4 and add a spkg-postrm script to clean the documentation.
A reviewer should validate ​829317e in order to set this ticket in positive review.

[videlec]

comment:209

doing some testing...

[videlec]

comment:210

spkg-postrm works as expected. Thanks Vincent!

[slel]

comment:211

Adding "upgrade" to keywords to emphasize the need to upload new tarball to our download mirrors.

[slel]

Changed keywords from thursdaysbdx, ppl, pplpy to thursdaysbdx, ppl, pplpy, upgrade

[vbraun]

Changed branch from u/vklein/replacement_of_sage_libs_ppl_pyx_by_pplpy to 829317e

[novoselt]

comment:213

There were several instances of removal similar to self._PPL_C_Polyhedron.set_immutable() - is this functionality no longer supported? This was done for objects that are cached and if a user ever modifies the polyhedron, things will get wrong.

[novoselt]

Changed commit from 829317e to none

[videlec]

comment:214

Replying to @novoselt:

There were several instances of removal similar to self._PPL_C_Polyhedron.set_immutable() - is this functionality no longer supported? This was done for objects that are cached and if a user ever modifies the polyhedron, things will get wrong.

Indeed, it is not supported anymore. Also you can not cache a PPL_C_Polyhedron: https://gitlab.com/videlec/pplpy/blob/master/ppl/polyhedron.pyx#L86-102

In sage, the pplpy polyhedra are only used as attribute from which the user is not supposed to have direct access (unless she knows very well what she is doing). The higher level Polyhedron_base still implements this mutability flag.

Could you give a concrete example of a problematic behavior?

[novoselt]

comment:215

Well I don't have an example from "real life" given that these objects used to be immutable and that I generally knew what I am doing, but the situation I have described is not at all impossible. Although the method returning PPL representation is underscored. There is also a discrepancy with documentation at
https://github.com/sagemath/sage-prod/blob/develop/src/sage/geometry/cone.py#L1373
which claims that supplied polytope will be made immutable.

[videlec]

comment:216

Please continue the discussion at #27423 since this ticket is closed.

[jdemeyer]

Changed reviewer from Dima Pasechnik, François Bissey, ​Vincent Delecroix, Jeroen Demeyer, Erik Bray to Dima Pasechnik, François Bissey, Vincent Delecroix, Jeroen Demeyer, Erik Bray