Replacement of sage/libs/ppl.pyx by pplpy

[vklein]

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

[git]

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

​f8bf274

Updating checksum for pplpy pkg

[vklein]

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

[vdelecroix]

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.

[vdelecroix]

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

[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

[vklein]

Replying to vdelecroix:

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

[git]

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

​b3563eb

remove mpz_to_sage_integer function

[vdelecroix]

Replying to vklein:

Replying to vdelecroix:

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?

[vklein]

Replying to vdelecroix:

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.

[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

[vklein]

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

[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

[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

[vklein]

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

[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

[vdelecroix]

​pplpy 0.7 is out

[git]

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

​760bbdf	Modify associahedron doctest
​6d3345e	Updating checksum for pplpy pkg
​85cb9fc	remove mpz_to_sage_integer function
​70177c8	assert simplification ppl_lattice_polytope.py
​b0858a2	remove skip=('_test_pickling') option for pplpy polyhedron
​94c7817	remove sage/libs/ppl from doctree
​67c8c2e	update checksum, snapshot version update in setup.py
​50d62ff	Add path of gmpy2 in include_dirs
​375fda6	Trac #23024 Fix checksum (pplpy), spkg-install and spkg-check
​dc37f38	Trac #23024 Add new mpz gmpy2 to sage Integer conversion

[vklein]

Rebase on 22928 (with 22928 based on 8.2.beta0)

[vdelecroix]

You need to provide a link to the pplpy tarball.

[vdelecroix]

And proper deprecation has to be done for sage.libs.ppl so that a user doing

sage: from sage.libs.ppl import whatever

should not get an ImportError but a warning. See ​http://doc.sagemath.org/html/en/reference/misc/sage/misc/superseded.htm

[vklein]

And by proper deprecation do you mean keeping sage.libs.ppl fully functioning with his doctests ? Or do we prefer to shutdown sage.libs.ppl functionalities (Functions don't do their old work and just return a warning) ?

[vdelecroix]

Replying to vklein:

And by proper deprecation do you mean keeping sage.libs.ppl fully functioning with his doctests ? Or do we prefer to shutdown sage.libs.ppl functionalities (Functions don't do their old work and just return a warning) ?

The aim is not to keep sage.libs.ppl but to just to have some indirection in the import so that you do not break user's code. As sage.libs.ppl will not here anymore I do not expect the full doctesuite to pass as it used to be. But the following kind of things should work (and be doctested)

sage: from sage.libs.ppl import Variable, Constraint_System, C_Polyhedron
UserWarning: proper deprecation message
sage: x = Variable(0)
sage: y = Variable(1)
sage: cs = Constraint_System()
sage: cs.insert(x >= 0)
sage: cs.insert(x <= 3)
sage: cs.insert(y >= 0)
sage: cs.insert(y <= 3)
sage: poly_from_constraints = C_Polyhedron(cs)

And the various objects constructed above are of course from pplpy.

You can have a look at other places in the sage code and look for "deprecation" and "lazy_import".

[vklein]

Well, i spoke with Sebastien L. and if the goal is to let one year for developers to adapt their old code, doing an indirection is not enough. Mainly because pplpy use gmpy2 (some functions return and calls are differents).

Therefore it seems like we should keep sage.libs.ppl old code with added warnings.

[fbissey]

Just a small request can you put upstream's address in SPKG.txt? Of course you are available on pypi but it is not the same as being able to find where to fill bugs and what the current issues are straight away.

[git]

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

​7d19154

Trac #23024 SPKG.txt add upstream contact and licence.

[vklein]

Done, thanks for noticing that.

[git]

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

​01403bd

Trac #23024 Deprecate sage.libs.ppl

[vklein]

Deprecation of sage.libs.ppl added. As the whole module is deprecated i choose to send deprecation warning at import time.

[vdelecroix]

Replying to vklein:

Deprecation of sage.libs.ppl added. As the whole module is deprecated i choose to send deprecation warning at import time.

Good solution.

Now, we just need to wait for #22928...

[vdelecroix]

Did you run the testsuite!?

sage -t src/sage/geometry/lattice_polytope.py
**********************************************************************
File "src/sage/geometry/lattice_polytope.py", line 1608, in sage.geometry.lattice_polytope.LatticePolytopeClass.boundary_point_indices
Failed example:
    face.points()
Exception raised:
    Traceback (most recent call last):
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.geometry.lattice_polytope.LatticePolytopeClass.boundary_point_indices[4]>", line 1, in <module>
        face.points()
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/geometry/lattice_polytope.py", line 3680, in points
        l = gcd(v)
    NameError: global name 'gcd' is not defined

[git]

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

​e395e0e	Trac #23024 Use gmpy2 as a standard package
​d244109	Trac #23024 Restore import gcd

[vklein]

Yes i have see this one. The last remaining problem is a SEGFAULT when running ppl_lattice_polytope.py doctests.

[vdelecroix]

Could you post the log here?

[vklein]

Replying to vdelecroix:

Could you post the log here?

​segfault_ppl_lattice_polytope.log

Short story is

sage: LatticePolytope_PPL((0,0),(1/2,1))

cause a Segfault.

[vdelecroix]

Isn't it ​https://github.com/aleaxit/gmpy/pull/174?

[vdelecroix]

At least with the patch applied I got

sage: from sage.geometry.polyhedron.ppl_lattice_polytope import LatticePolytope_PPL
sage: LatticePolytope_PPL((0,0),(1/2,1))
Traceback (most recent call last):
...
TypeError: rational is not an integer

[vdelecroix]

See #24417

[vklein]

Replying to vdelecroix:

Isn't it ​https://github.com/aleaxit/gmpy/pull/174?

Yes it is ! Nicely done.

[git]

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

​7596cff	remove sage/libs/ppl from doctree
​0cd326c	update checksum, snapshot version update in setup.py
​91b5d52	Add path of gmpy2 in include_dirs
​0b965a7	Trac #23024 Fix checksum (pplpy), spkg-install and spkg-check
​dd886fb	Trac #23024 Add new mpz gmpy2 to sage Integer conversion
​b7a5a21	Trac #23024 SPKG.txt add upstream contact and licence.
​96146ca	Trac #23024 Deprecate sage.libs.ppl
​e85ae5f	Trac #23024 Use gmpy2 as a standard package
​da9aa35	Trac #23024 Restore import gcd
​3cec632	Trac #23024 : merge #22928 ppl.pyx add deprecation warning ...

[git]

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

​d206c14

Trac #23024: ppl.pyx add deprecation warning to long doctest

[vdelecroix]

You are adding sage.libs.gmpy2 in 7596cff and removing it in 91b5d52...

[vklein]

Yes adding it was one of our old solutions. Before having a better include in pplpy's setup.py if i remember correctly. All of this historical things can be squashed later.

[vklein]

It's the same for ppl.pyx we don't need to show that we deleted it and restore and deprecate later.

[vdelecroix]

There are a lot of trailing whitespaces that should be removed.

[git]

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

​886424b	Trac 23024: Add pplpy as a standard package
​a6e1cc7	Trac 23024: Replace sage.libs.ppl with pplpy package
​f3e4bcd	Trac #23024: Use gmpy2 as a standard package

[vklein]

History rewrited. Trailing whitespaces removed in real_mpfr.pyx, complex_mpc.pyx and complex_number.pyx files.

[vdelecroix]

I don't understand why you are modifying pplpy/SPKG.txt, pplpy/checksums.ini, pplpy/spkg-check, etc in both 886424b and ​a6e1cc7.

[vdelecroix]

Build fine and all tests pass on my computer!

[jdemeyer]

Use sdh_pip_install instead of $PIP_INSTALL

[jdemeyer]

Is the license of pplpy really GPL version 3? Or do you mean GPL version 3 or later? The difference is important.

[jdemeyer]

This looks like a strange way to check if something is one:

Integer(p.divisor()).is_one()

What's wrong with

p.divisor() == 1

[vklein]

Replying to jdemeyer:

Is the license of pplpy really GPL version 3? Or do you mean GPL version 3 or later? The difference is important.

Yes the current one is GPL version 3 (see ​LICENSE.txt). Is it better to say GPL version 3 or later anyway ?

[jdemeyer]

In backend_ppl.py you do from gmpy2 import mpz but you're not using it.

[jdemeyer]

Replying to vklein:

Is it better to say GPL version 3 or later anyway ?

See ​https://github.com/videlec/pplpy/issues/28

[vklein]

Replying to jdemeyer:

What's wrong with

p.divisor() == 1

p.divisor() return a gmpy2.mpz.

[git]

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

​ef97c32

Trac #23024: spkg-install `sdh_pip_install` instead ...

[vdelecroix]

Replying to vklein:

Replying to jdemeyer:

What's wrong with

p.divisor() == 1

p.divisor() return a gmpy2.mpz.

What is wrong with

>>> gmpy2.mpz(1) == 1
True

[vklein]

Replying to vdelecroix:

... What is wrong with

>>> gmpy2.mpz(1) == 1
True

In this case you compare a python int with a gmpy2.mpz.

But with sage's Integer :

sage: gmpy2.mpz(1) == 1
False

Edit: Hum ok my bad.

[git]

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

​2f98d70

Trac #23024: Code cleaning

[dimpase]

self-testing fails:

$ SAGE_CHECK=yes ./sage -f pplpy
...
[pplpy-0.7] Successfully installed pplpy-0.7
[pplpy-0.7] Cleaning up...
[pplpy-0.7] 
[pplpy-0.7] real	0m38.345s
[pplpy-0.7] user	0m36.490s
[pplpy-0.7] sys	0m1.835s
[pplpy-0.7] Copying package files from temporary location /home/dima/Sage/sage-dev/local/var/tmp/sage/build/pplpy-0.7/inst to /home/dima/Sage/sage-dev/local
[pplpy-0.7] Running the test suite for pplpy-0.7...
[pplpy-0.7] running test
[pplpy-0.7] error: cannot copy tree './tests': not a directory
[pplpy-0.7] 

[vklein]

Indeed. ​https://github.com/videlec/pplpy/issues/30

[embray]

This would still be a good thing to do IMO, in general.

[git]

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

​f5b7110	Trac 23024: Add pplpy as a standard package
​82a5d24	Trac 23024: Replace sage.libs.ppl with pplpy package
​59617e1	Trac #23024: Use gmpy2 as a standard package
​8f51887	Trac #23024: spkg-install `sdh_pip_install` instead ...
​d7a67d4	Trac #23024: Code cleaning
​9ea9382	Trac #23024: Remove HAVE_GMPY2 from coerce.pyx
​55ad161	Trac #23024: fix ppl_lattice_polytope doctests

[vklein]

Rebased on sage8.6

[git]

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

​90d999b	Trac #23024: fix ppl_lattice_polytope doctests
​e90bc77	Trac #23024: upgrade pplpy version to 0.8.1

[vklein]

upgrade pplpy to 0.8.1

[vdelecroix]

Apparently a merge failure with sage 8.7.beta0

[git]

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

​c37dcb0	Trac 23024: Add pplpy as a standard package
​3f5eed8	Trac 23024: Replace sage.libs.ppl with pplpy package
​26d3d54	Trac #23024: Use gmpy2 as a standard package
​1a5d7e5	Trac #23024: spkg-install `sdh_pip_install` instead ...
​62452d5	Trac #23024: Code cleaning
​58a0e8a	Trac #23024: Remove HAVE_GMPY2 from coerce.pyx
​dabd865	Trac #23024: fix ppl_lattice_polytope doctests
​9a06541	Trac #23024: upgrade pplpy version to 0.8.1

[vklein]

Solve merge failure with sage 8.7.beta0.

[dimpase]

• off-topic - Vincent, please provide info on

​https://github.com/OpenDreamKit/OpenDreamKit/issues/281

[vdelecroix]

You should set gmpy2 and pplpy as dependencies of the Sage library (otherwise building the Cython code might fail). This is done in build/make/deps.

[vdelecroix]

Why

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

[vdelecroix]

And

-    def _repr_(self):
+    def __repr__(self):

[vdelecroix]

The links in the documentation :class:`ppl.C_Polyhedron`, :class:`~ppl.Polyhedron`, etc are broken. These should not be sphinx links at all.

[dimpase]

Does pplpy build/install its own docs? This kind of setup is used e.g. in cvxopt.

[vdelecroix]

Replying to dimpase:

Does pplpy build/install its own docs? This kind of setup is used e.g. in cvxopt.

It does build documentation if you ask it to. Tough it does not install anything (perhaps it should?). But even with properly installed documentation, I don't see how to make linking work properly.

[dimpase]

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

[vklein]

Replying to vdelecroix:

And

-    def _repr_(self):
+    def __repr__(self):

That's because LatticePolytope_PPL_class now inherit from pplpy's C_Polyhedron and therefore LatticePolytope_PPL_class is not a SageObject.

[vklein]

Replying to dimpase:

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

I can try.
But i can see a potential problem here: most packages seems to install documentation on if [[ "$SAGE_SPKG_INSTALL_DOCS" = yes ]].
If we do the same for pplpy, given $SAGE_SPKG_INSTALL_DOCS default value is not yes that mean that the links will be broken by default.

I think there is many distinct questions :

• 1. Should we install a pplpy documentation ?
• 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?
• 3. If yes to 1. and yes to 2. should we link the sagemaths documentation with pplpy's.

[git]

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

​13507e4

Trac #23024: Implement reviewer's comments : add depende...

[vdelecroix]

It is a good practice to have the first line of the commit below 50 characters. More precisely, any commit message should be

one line with <= 50 characters
(optional) linebreak
(optional) more extensive paragraph with lines <= 72 characters

[vdelecroix]

Replying to vklein:

Replying to dimpase:

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

I can try.
But i can see a potential problem here: most packages seems to install documentation on if [[ "$SAGE_SPKG_INSTALL_DOCS" = yes ]].
If we do the same for pplpy, given $SAGE_SPKG_INSTALL_DOCS default value is not yes that mean that the links will be broken by default.

I think there is many distinct questions :

• 1. Should we install a pplpy documentation ?
• 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?
• 3. If yes to 1. and yes to 2. should we link the sagemaths documentation with pplpy's.

I don't know what is the way to proceed. See ​https://gitlab.com/videlec/pplpy/issues/12

For now I would say:

• remove all links in the Sage documentation
• in each relevant module, make a reference to the pplpy development website (​https://gitlab.com/videlec/pplpy) and pplpy documentation website (​http://www.labri.fr/perso/vdelecro/pplpy/latest/).

[dimpase]

I've left comments in ​https://gitlab.com/videlec/pplpy/issues/12 namely there appears to be a Sphinx extension to link different projects.

It's too early to start blindly removing links to docs, probably there is a more intelligent way to preserve what is already there.

[vdelecroix]

Replying to vklein:

Replying to dimpase:

Packages install docs into $SAGE_LOCAL/share/doc/. I guess that docs can be linked to docs in Sage code via relative paths, i.e. ../../..

I can try.
But i can see a potential problem here: most packages seems to install documentation on if [[ "$SAGE_SPKG_INSTALL_DOCS" = yes ]].
If we do the same for pplpy, given $SAGE_SPKG_INSTALL_DOCS default value is not yes that mean that the links will be broken by default.

I think there is many distinct questions :

• 1. Should we install a pplpy documentation ?

We should compile and install.

• 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?

I don't know. Where is SAGE_SPKG_INSTALL_DOCS documented?

• 3. If yes to 1. and yes to 2. should we link the sagemaths documentation with pplpy's.

See #27164.

[vklein]

Replying to vdelecroix:

...

• 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?

I don't know. Where is SAGE_SPKG_INSTALL_DOCS documented?

Here ​http://doc.sagemath.org/html/en/installation/source.html.

SAGE_SPKG_INSTALL_DOCS - if set to yes, then install package-specific documentation to $SAGE_ROOT/local/share/doc/PACKAGE_NAME/ when an spkg is installed. This option may not be supported by all spkgs. Some spkgs might also assume that certain programs are available on the system (for example, latex or pdflatex).

[git]

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

​665419a

Trac #23024: Implement reviewer's comments ...

[vdelecroix]

Replying to vklein:

Replying to vdelecroix:

...

• 2. If yes should we install it only if "$SAGE_SPKG_INSTALL_DOCS" = yes ?

I don't know. Where is SAGE_SPKG_INSTALL_DOCS documented?

Here ​http://doc.sagemath.org/html/en/installation/source.html.

SAGE_SPKG_INSTALL_DOCS - if set to yes, then install package-specific documentation to $SAGE_ROOT/local/share/doc/PACKAGE_NAME/ when an spkg is installed. This option may not be supported by all spkgs. Some spkgs might also assume that certain programs are available on the system (for example, latex or pdflatex).

I believe it has been done that way a long time ago when Sage was not thought modular at all. Some of the package documentation (including pplpy) might be automatically installed unless SAGE_SPKG_INSTALL_DOCS is explicitely set to "no".

[git]

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

​ef89978

Trac #23024: Generate and installl doc

[vklein]

For testing purpose use ​https://www.labri.fr/perso/vklein/pplpy-0.8.2b0.tar.gz with ef89978.

[vklein]

ef89978 Implements the compilation and installation of pplpy's documentation.

The current implementation drawback is that pplpy is compiled two times (one time for package installation and one time using "inplace" build in preparation for sphinx).

To avoid that i am looking for an elegant and generic way to express PYTHONPATH=$PYTHONPATH:$SAGE_DESTDIR_LOCAL/lib/python2.7/site-packages.

I haven't find an environment variable for lib/python2.7/site-packages or something equivalent.

Any help is welcomed.

[git]

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

​d8c41f6

Trac #23024: Add intersphinx between sage doc ...

[vklein]

d8c41f6 Fix intersphinx question.

I wait for returns for ​#comment:106 and for a new pplpy official version before setting this ticket in need_review.

[dimpase]

I wonder whether you tried to set up a link to pplpy docs in Sage's reference manual, something that ought to be done for many more packages...

[vklein]

Replying to dimpase:

I wonder whether you tried to set up a link to pplpy docs in Sage's reference manual, something that ought to be done for many more packages...

I am not sure what is your question about.

I have think about doing the intersphinx link at reference level yes. But IMHO doing it under common is a better call.

I haven't tried to do direct html link with relative path.

[dimpase]

I meant to ask whether you managed to replace the docs link to PPL backend in the reference manual, such as ​http://doc.sagemath.org/html/en/reference/discrete_geometry/sage/geometry/polyhedron/backend_ppl.html (its local equivalent would have something else instead of http://doc.sagemath.org/ part, naturally) with the link to the pplpy docs.

[vdelecroix]

Crossed links does not work for me (e.g. the ppl.C_Polyhedron in the file en/reference/discrete_geometry/sage/geometry/polyhedron/ppl_lattice_polytope.html) ... did you do something special in order to make it work for you?

[vklein]

Replying to vdelecroix:

Crossed links does not work for me (e.g. the ppl.C_Polyhedron in the file en/reference/discrete_geometry/sage/geometry/polyhedron/ppl_lattice_polytope.html) ... did you do something special in order to make it work for you?

Maybe try a make doc-clean. `ppl.C_Polyhedron` should have been replaced by `ppl.polyhedron.C_Polyhedron`.

[vdelecroix]

Replying to vklein:

Replying to vdelecroix:

Crossed links does not work for me (e.g. the ppl.C_Polyhedron in the file en/reference/discrete_geometry/sage/geometry/polyhedron/ppl_lattice_polytope.html) ... did you do something special in order to make it work for you?

Maybe try a make doc-clean. `ppl.C_Polyhedron` should have been replaced by `ppl.polyhedron.C_Polyhedron`.

I did make doc-clean, sage -f pplpy and make doc.

[vdelecroix]

I know what I did wrong: I did not pull your last commits... sorry for the noise.

[vdelecroix]

It is working now!

[vdelecroix]

I will make a new release of pplpy.

Could you ask about 106 on sage-devel? This is a general problem when a package needs to be installed before being able to build the docs. Perhaps this is a problem with the package itself or in the way our current staged installation works in Sage. But then, it is not clear to me how to solve it.

[vklein]

For comment:106 : link to the post on sage-devel ​https://groups.google.com/forum/#!topic/sage-devel/IOn7wneT8xo

[vklein]

Replying to dimpase:

I meant to ask whether you managed to replace the docs link to PPL backend in the reference manual, such as ​http://doc.sagemath.org/html/en/reference/discrete_geometry/sage/geometry/polyhedron/backend_ppl.html (its local equivalent would have something else instead of http://doc.sagemath.org/ part, naturally) with the link to the pplpy docs.

I managed to replace the doc links of type sage.libs.ppl.*. But there is no such kind of doclinks under backend_ppl.html. Class like sage.geometry.polyhedron.base_QQ.Polyhedron_QQ are still sage classes.

[vdelecroix]

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

[dimpase]

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

-1. I'd rather run the installation twice than this.

[vklein]

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

Why not. It will increase the tarball size of something like 200 kbytes.

[vklein]

Replying to dimpase:

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

-1. I'd rather run the installation twice than this.

Why ? (I personally have no preferences between the two solutions).

[dimpase]

Replying to vklein:

Replying to vdelecroix:

Alternative: include the compiled doc in the source tarball so that the spkg-install script just need to copy it...

Why not. It will increase the tarball size of something like 200 kbytes.

The external tarball can contain extra 200K, sure, but already "just copying" has to be done properly, in order not to break package de-installation. Besides, there is no guarantee it would look nice enough, so that it matches the current Sage docs style, etc.

[vklein]

Ok i see. And i agree that having the same sphinx install generating both documentation is a good thing.

[git]

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

​41e16e6	Trac #23024: Use gmpy2 as a standard package
​68e0e45	Trac #23024: spkg-install `sdh_pip_install` instead ...
​9da630f	Trac #23024: Code cleaning
​273b772	Trac #23024: Remove HAVE_GMPY2 from coerce.pyx
​1365deb	Trac #23024: fix ppl_lattice_polytope doctests
​3da5acf	Trac #23024: upgrade pplpy version to 0.8.1
​9fed2a3	Trac #23024: Implement reviewer's comments ...
​33f4759	Trac #23024: Generate and installl doc
​3cf578c	Trac #23024: Add intersphinx between sage doc ...
​c40f656	Trac #23024: Upgrade to pplpy version 8.2

[vklein]

Upgrade pplpy to 8.2.
Rebase on sage 8.7.beta1.

For the doc generation, i think we can go with the double compilation now and switch
to a better solution later if we find one.

[git]

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

​bcb1373	Trac #23024: Add pplpy as a standard package
​1aec143	Trac #23024: Replace sage.libs.ppl with pplpy package
​3231e47	Trac #23024: Use gmpy2 as a standard package
​93e0b66	Trac #23024: Doc and dependencies: ...

[vklein]

Rewrite ticket history.

[vdelecroix]

To my mind, a better solution has to be found for

+    # Compile pplpy's documentation
+    sage-python23 setup.py build_ext --inplace
+    PYTHONPATH=$PYTHONPATH:$(pwd)
+    cd docs
+    $MAKE html

[dimpase]

As long as gmpy2 is optional, this package cannot be standard, as it depends on gmpy2.

(Edit - I misread the ticket description, hence the edit). I recall last time there was a discussion about gmpy2 being standard, there were quite a bit of objections. Do they still stand?

[vklein]

Yes, this is the discussion link ​https://groups.google.com/forum/?#!topic/sage-devel/qoxVng1__0A.

The discussion/objections where more about what types pplpy should return (Sage numbers or gmpy2 numbers).

The last 3 messages for gmpy2 solution have never been answered, but rereading the thread it's true that it doesn't mean that we have an explicit consensus.

I put this ticket on hold in wait for better solution for generating documentation.

I will repost on the discussion thread when this ticket is ready.

[dimpase]

Replying to vdelecroix:

To my mind, a better solution has to be found for

+    # Compile pplpy's documentation
+    sage-python23 setup.py build_ext --inplace
+    PYTHONPATH=$PYTHONPATH:$(pwd)
+    cd docs
+    $MAKE html

the question is - are you prepared to patch pplpy sources? Comparing their docs/Makefile with ​https://github.com/sagemath/sagenb/blob/master/doc/Makefile seems to say that there are sphinx options one might try to use to do everything in one build, not in two.

[vklein]

I haven't find a decisive difference between the two Makefile. The most obvious one being that pplpy use sphinx-build -b and sagenb use sphinx-build -M.

I have tried with -M and get the same result.
I think the reason it works with sagenb is because it doesn't have cython files and therefore the source directory is enough for the sphinx build to succeed.

[git]

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

​e2d3e07	Trac #23024: Add pplpy as a standard package
​ba656ae	Trac #23024: Replace sage.libs.ppl with pplpy package
​5ac4d45	Trac #23024: Use gmpy2 as a standard package
​c4a7255	Trac #23024: Doc and dependencies: ...
​8d020ea	Trac #23024: Sphinx doc avoid to compile two ...

[vklein]

Add a workaround to avoid two build and rebase on 8.7.beta3.

[vdelecroix]

It is not working for me

[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl'; the following exception was raised:
[pplpy-0.8.2] No module named ppl
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.constraint'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.constraint
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.linear_algebra'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.linear_algebra
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.generator'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.generator
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.polyhedron'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.polyhedron
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.mip_problem'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.mip_problem

[vdelecroix]

And shouldn't python be python23?

[vdelecroix]

And the command you provided is very suspicious to me. Inside the sage shell

$ python -c "import site; print(site.getusersitepackages().replace(site.getuserbase(), ''))"
/home/vincent/.sage/local/lib/python2.7/site-packages

And using string replace is not an option

>>> import site
>>> site.getusersitepackages()
'/home/vincent/.sage/local/lib/python2.7/site-packages'
>>> site.getuserbase()
'/home/vincent/.sage//local'

[vdelecroix]

This already looks more reasonable

>>> import sysconfig
>>> sysconfig.get_paths(expand=False)
{'purelib': '{base}/lib/python{py_version_short}/site-packages',
 'stdlib': '{base}/lib/python{py_version_short}',
 'scripts': '{base}/bin',
 'platinclude': '{platbase}/include/python{py_version_short}',
 'include': '{base}/include/python{py_version_short}',
 'data': '{base}', 'platstdlib':
 '{platbase}/lib/python{py_version_short}',
 'platlib': '{platbase}/lib/python{py_version_short}/site-packages'
}

[vdelecroix]

I believe that what you want is

>>> import sysconfig
>>> py_version_short = sysconfig.get_config_vars()['py_version_short']
>>> path = sysconfig.get_path('platlib', expand=False)
>>> path.format(platbase="$SAGE_DESTDIR_LOCAL", py_version_short=py_version_short)
'$SAGE_DESTDIR_LOCAL/lib/python2.7/site-packages'

[vklein]

the site module is already supposed to do the job in a generic way :

def _get_path(userbase):
    version = sys.version_info

    if os.name == 'nt':
        return f'{userbase}\\Python{version[0]}{version[1]}\\site-packages'

    if sys.platform == 'darwin' and sys._framework:
        return f'{userbase}/lib/python/site-packages'

    return f'{userbase}/lib/python{version[0]}.{version[1]}/site-packages'

Python version is not always used

[git]

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

​4b462ab

Trac #23024: Sphinx doc avoid to compile two ...

[vklein]

Replying to vdelecroix:

And shouldn't python be python23?

Yes you are right.
Workaround fixed and enhanced with Erik Bray's help.

[vdelecroix]

Same as before

[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl'; the following exception was raised:
[pplpy-0.8.2] No module named ppl
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.constraint'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.constraint
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.linear_algebra'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.linear_algebra
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.generator'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.generator
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.polyhedron'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.polyhedron
[pplpy-0.8.2] WARNING: autodoc: failed to import module u'ppl.mip_problem'; the following exception was raised:
[pplpy-0.8.2] No module named ppl.mip_problem

[vklein]

Can you post or mail me the whole log ?

[vklein]

Ok i have an obvious bug in my spgk_install file

[git]

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

​f4fe72b

Trac #23024: Sphinx doc avoid to compile two ...

[vklein]

It's fixed sorry for the previous commit.

[vdelecroix]

Why does sphinx inventory connect to internet!?

[pplpy-0.8.2] loading intersphinx inventory from http://docs.python.org/objects.inv...

[vklein]

I believe because of pplpy's conf.py last line :

# Example configuration for intersphinx: refer to the Python standard library.
intersphinx_mapping = {'http://docs.python.org/': None}

This says pplpy's intersphinx needs python's objects.inv.

Wich isn't true if i am not wrong ( I don't remember a sphinx link to python documentation in pplpy's documentation)

[vdelecroix]

new tarball ready

[git]

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

​abaef5e

Trac #23024: upgrade pplpy to version 0.8.3

[vklein]

Ticket ready for review.

Don't set this ticket to positive review right now.
We need to clarify the discussion ​https://groups.google.com/forum/?#!topic/sage-devel/qoxVng1__0A first.

[vdelecroix]

Replying to vklein:

Don't set this ticket to positive review right now.
We need to clarify the discussion ​https://groups.google.com/forum/?#!topic/sage-devel/qoxVng1__0A first.

What is the status of the discussion now?

[vdelecroix]

Note: for the documentation build, there is also the possibility to use the spkg-postinst script (that is run after the installation in $SAGE_LOCAL).

[vklein]

Replying to vdelecroix:

Note: for the documentation build, there is also the possibility to use the spkg-postinst script (that is run after the installation in $SAGE_LOCAL).

Yes that's true. The reason to keep the documentation build in spkg-inst is because it's currently the standard way of doing it. Tell me what you prefer.

[vdelecroix]

Replying to vklein:

Replying to vdelecroix:

Note: for the documentation build, there is also the possibility to use the spkg-postinst script (that is run after the installation in $SAGE_LOCAL).

Yes that's true. The reason to keep the documentation build in spkg-inst is because it's currently the standard way of doing it. Tell me what you prefer.

To my knowledge, no package had this problem before (ie building the doc depending on the library being installed). Everything that would avoid modifying PYTHONPATH looks nicer to me. But that is my own feeling.

[git]

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

​c6d75a9

Trac #23024: Add a spkg-postinst script to man...

[vklein]

Ok seems fair. spkg-postinst script added.

[vdelecroix]

Having a comment in the spkg-install script is good but I think that it would be even nicer with an explanation of why this is moved inside the spkg-postinst script.

[git]

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

​794aed8

Trac #23024: spkg-install: Document the reason ...

[vklein]

Ok, explanation added.

[vdelecroix]

The current status is good for me. Thanks Vincent.

Any comment Dima?

[dimpase]

How about src/sage/numerical/backends/ppl_backend.pyx ? Do you at least have a ticket to work on replacing this with pplpy?

[vklein]

ppl_backend.pyx is modified in this ticket. This is not replaced but ppl_backend.pyx methods use pplpy instead of using sage.lib.ppl.

see: ​https://git.sagemath.org/sage.git/diff/src/sage/numerical/backends/ppl_backend.pyx?id=fed0a135795f8f076d7ffaf69aecc84f2f096b5d

[vklein]

@Dima tell me if you think it's ok or if you think ppl_backend.pyx should be reworked in another way.

[dimpase]

I recall that ppl_backend.pyx had a tricky part due to the inability of PPL to work with rationals - everything had to be integer. I don't know whether this is still the case. Potentially, if it, or/and pplpy can now work directly with rationals, it might be worthwhile to change, as computing common denominators lead to blowup of coefficients.

[vdelecroix]

Replying to dimpase:

I recall that ppl_backend.pyx had a tricky part due to the inability of PPL to work with rationals - everything had to be integer. I don't know whether this is still the case. Potentially, if it, or/and pplpy can now work directly with rationals, it might be worthwhile to change, as computing common denominators lead to blowup of coefficients.

The PPL Coefficient type is hardcoded in the Cython wrapper as

ctypedef mpz_class PPL_Coefficient

This implies that all computations with pplpy are done with GMP mpz. I agree that PPL supports computations with mpz_class, mpq_class as well as various native integer types (from char to long long). Nothing has changed on this side yet. This feature request is disjoint from this ticket which aims to be a simple transition from Sage source code to standalone library.

[dimpase]

looks good to me then.

[vdelecroix]

Replying to vdelecroix:

Replying to dimpase:

I recall that ppl_backend.pyx had a tricky part due to the inability of PPL to work with rationals - everything had to be integer. I don't know whether this is still the case. Potentially, if it, or/and pplpy can now work directly with rationals, it might be worthwhile to change, as computing common denominators lead to blowup of coefficients.

The PPL Coefficient type is hardcoded in the Cython wrapper as

ctypedef mpz_class PPL_Coefficient

This implies that all computations with pplpy are done with GMP mpz. I agree that PPL supports computations with mpz_class, mpq_class as well as various native integer types (from char to long long). Nothing has changed on this side yet. This feature request is disjoint from this ticket which aims to be a simple transition from Sage source code to standalone library.

This is high priority and I opened an issue: ​https://gitlab.com/videlec/pplpy/issues/14

[vklein]

Thanks to all reviewers.

[vbraun]

merge conflict

[git]

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

​477a7be	Trac #23024: Add pplpy as a standard package
​44b61c5	Trac #23024: Replace sage.libs.ppl with pplpy package
​a4c722c	Trac #23024: Use gmpy2 as a standard package
​684e107	Trac #23024: Doc and dependencies: ...
​b49007c	Trac #23024: Sphinx doc avoid to compile two ...
​bfc737a	Trac #23024: upgrade pplpy to version 0.8.3
​b7e3547	Trac #23024: Add a spkg-postinst script to man...
​81bf994	Trac #23024: spkg-install: Document the reason ...

[vklein]

Rebase on 8.7.beta5

[jdemeyer]

Looking at the diff, I have several minor comments:

1. Don't change whitespace in src/module_list.py for no good reason.

2. In various places, you use list(...). Why not use a list comprehension instead?

3. In multi-line imports, the preferred style is to use parentheses instead of \.

4. The deprecation message is really strangely formatted: it shouldn't start with a newline and there shouldn't be a huge amount of indentation in the second line. I would probably write it all on one line without any newlines at all.

5. What's the meaning of the /... at the start of the deprecation warnings in doctests? I have never seen that.

6. In _add_rational_constraint, why do you cdef Rational rhs? Since numerator and denominator are pure Python methods, this is a pessimization.

[embray]

Replying to jdemeyer:

Looking at the diff, I have several minor comments:

2. In various places, you use list(...). Why not use a list comprehension instead?

I wonder if there shouldn't be something like this mentioned in the developer docs (do we have a style guide)?

This is a comment I've made in review on dozens of tickets at this point.

Short list comprehensions (it's trickier when they get nested) are often clearer than any alternative, and are usually much more efficient from the Python interpreter's standpoint as well.

[vklein]

5. What's the meaning of the /... at the start of the deprecation warnings in doctests? I have never seen that.

It means the line begin with / then something before DeprecationWarning.... I think i have done that because the print(err) result begin by the path $SAGE_LOCAL/src/bin/sage-eval.

[jdemeyer]

I see now. Those aren't "normal" doctests, they are actually run in a subprocess.

[git]

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

​b25b719

Trac #23024: Implements reviewer's comment :

[vklein]

Comments 1,2,3,4 and 6 implemented.

[vdelecroix]

Jeroen?

[jdemeyer]

Good for me if this passes doctests (I haven't checked)

[vdelecroix]

Replying to jdemeyer:

Good for me if this passes doctests (I haven't checked)

This I did (and I belive Vincent K also)

[vklein]

After the last commit i only did tests on all modified files and everything under sage/geometry. I will launch a sage -t -a --long just to be extra sure.

[embray]

I would like to test this on Windows. I can do that in a few minutes.

[embray]

One minor comment just looking at the patch: To complement the pplpy/spkg-postinst script, it would be good, albeit not strictly necessary, to include a pplpy/spkg-postrm script that removes that documentation directory in its entirety, if it was installed. That way it's a bit tidier if you also remove or upgrade pplpy and its documentation changes.

[vklein]

Ok, when exactly are spkg-postrm scripts called ? sage -f pkgname after the uninstall ?

Anyways i will wait for Owner field to be empty before pushing any new commit.

[embray]

Yeah, pplpy doesn't build on Cygwin:

[pplpy-0.8.3]     g++ -shared -Wl,--enable-auto-image-base -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o build/temp.cygwin-2.9.0-x86_64-2.7/ppl/ppl_shim.o -L/home/embray/src/sagemath/sage/local/lib/python2.7/config -L/home/embray/src/sagemath/sage/local/lib -lppl -lm -lpython2.7 -o build/lib.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.dll
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::clear()':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40661: undefined reference to `__imp___gmpz_set_ui'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40661:(.text+0x14c): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_set_ui'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::~Bit_Row()':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40618: undefined reference to `__imp___gmpz_clear'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40618:(.text+0x235): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_clear'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `__pyx_pf_3ppl_10bit_arrays_7Bit_Row___cinit__':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40588: undefined reference to `__imp___gmpz_init'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40588:(.text+0x2c9): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_init'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::clear_from(unsigned long)':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40639: undefined reference to `__imp___gmpz_tdiv_r_2exp'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40639:(.text+0x1dd0): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_tdiv_r_2exp'
[pplpy-0.8.3]     build/temp.cygwin-2.9.0-x86_64-2.7/ppl/bit_arrays.o: In function `Parma_Polyhedra_Library::Bit_Row::set(unsigned long)':
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40629: undefined reference to `__imp___gmpz_setbit'
[pplpy-0.8.3]     /home/embray/src/sagemath/sage/local/include/ppl.hh:40629:(.text+0x239d): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `__imp___gmpz_setbit'
[pplpy-0.8.3]     collect2: error: ld returned 1 exit status
[pplpy-0.8.3]     error: command 'g++' failed with exit status 1
[pplpy-0.8.3]     Running setup.py install for pplpy: finished with status 'error'

This looks...unsurprising to me. I think I can fix this easily-enough, I hope.

[embray]

Replying to vklein:

Ok, when exactly are spkg-postrm scripts called ? sage -f pkgname after the uninstall ?

Basically, at the end of sage-spkg-uninstall (which is called by sage -f among other things). This is also called when upgrading a package so that's why I thought it might matter eventually, at least in a minor way.

[embray]

Upstream PR: ​https://gitlab.com/videlec/pplpy/merge_requests/6

We can either see if Vincent wants to make an 0.8.4 release for this fix, or I'm just as happy for now to include this as a patch in the Sage SPKG.

[vdelecroix]

Replying to embray:

Upstream PR: ​https://gitlab.com/videlec/pplpy/merge_requests/6

We can either see if Vincent wants to make an 0.8.4 release for this fix, or I'm just as happy for now to include this as a patch in the Sage SPKG.

Let us do that.

[vdelecroix]

(the release 0.8.4)

[vdelecroix]

new release (ticket description updated)

[embray]

That was quick, thank you :)

Not sure yet if it's a showstopper, but I'm also having a weird problem with error handling in the gmpy2 in Sage. This following example should raise a TypeError exception:

$ python -c 'import gmpy2;  gmpy2.is_fermat_prp(1234, "a")'
Aborted (core dumped)

I also built the latest gmpy2 from source, and when I run it in that source tree I get the correct behavior:

$ python -c 'import gmpy2;  gmpy2.is_fermat_prp(1234, "a")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: is_fermat_prp() requires 2 integer arguments

Perhaps it was just a bug in gmpy2 that has since been fixed upstream? Not sure.

[embray]

Indeed, that particular bug was already fixed upstream: ​https://github.com/aleaxit/gmpy/commit/533a62cf2b2cf3673e1d6b0f3c2d0c64548a3908

I just wanted to make sure. I think I only hit on it because I was running the tests from gmpy2's current' master branch against our slightly older version, and hit that bug specifically because they added a regression test when they fixed it. Other than that all spkg-check tests pass on Cygwin for gmpy2 and pplpy :)

[embray]

All good on my end in terms of Sage doctests, I think. I'm doing one more full run just to be sure but I can think of no reason it should work differently.

So just update to the just-released pplpy 0.8.4 and you can set back to postive_review. The spkg-postrm would be good to have too but I don't consider it a blocker.

[vklein]

Ok i don't think doing a gmpy2 patch for this commit is worth it.

@Erik: Tell me when you're ok and i will update this ticket with pplpy's upgrade and spkg-postrm.

[embray]

Yep. Once you're done with that feel free to set back to positive_review.

[vklein]

Ok thank you for your help.

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

[vklein]

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.

[vdelecroix]

doing some testing...

[vdelecroix]

spkg-postrm works as expected. Thanks Vincent!

[slelievre]

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

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

[vdelecroix]

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]

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/blob/develop/src/sage/geometry/cone.py#L1373 which claims that supplied polytope will be made immutable.

[vdelecroix]

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