Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade pplpy to 0.8.9 #36161

Merged
merged 3 commits into from
Oct 8, 2023
Merged

Upgrade pplpy to 0.8.9 #36161

merged 3 commits into from
Oct 8, 2023

Conversation

videlec
Copy link
Contributor

@videlec videlec commented Aug 31, 2023

Upgrade the standard package pplpy to version 0.8.9 (released on PyPI https://pypi.org/project/pplpy/0.8.9/)

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 31, 2023

Lots of test failures

@videlec
Copy link
Contributor Author

videlec commented Sep 1, 2023

Lots of test failures

Indeed. I do not know what happens. It looks like that the Cython compilation happened with language_level=2.

@antonio-rojas
Copy link
Contributor

antonio-rojas commented Sep 2, 2023

Many of the failures have nothing to do with cython, it's an actual bug in pplpy introduced in sagemath/pplpy@5fa3580

More precisely, in the Linear_Expression constructor with two arguments, there is a bool check for the first argument. If this evaluates to false (like it happens, for instance, with a=Cone([(0,0)])._lattice(0), then space_dimension is never set, which is what is causing most of the test failures.

@videlec videlec changed the title Upgrade pplpy to 0.8.8 Upgrade pplpy to 0.8.9 Sep 2, 2023
@videlec
Copy link
Contributor Author

videlec commented Sep 2, 2023

Many of the failures have nothing to do with cython, it's an actual bug in pplpy introduced in sagemath/pplpy@5fa3580

More precisely, in the Linear_Expression constructor with two arguments, there is a bool check for the first argument. If this evaluates to false (like it happens, for instance, with a=Cone([(0,0)])._lattice(0), then space_dimension is never set, which is what is causing most of the test failures.

Indeed, bool([0, 0]) and bool(vector([0, 0])) behave differently! Thanks.

@antonio-rojas
Copy link
Contributor

The failures in sage/geometry/lattice_polytope.py, sage/geometry/polyhedron/backend_ppl.py, sage/geometry/polyhedron/ppl_lattice_polytope.py and the first failure in sage/geometry/cone.py are caused by sagemath/pplpy@3ed9df4, but not by the change in Cython language level, but rather by the __mul__ change in the Variable class

@videlec
Copy link
Contributor Author

videlec commented Sep 2, 2023

The failures in sage/geometry/lattice_polytope.py, sage/geometry/polyhedron/backend_ppl.py, sage/geometry/polyhedron/ppl_lattice_polytope.py and the first failure in sage/geometry/cone.py are caused by sagemath/pplpy@3ed9df4, but not by the change in Cython language level, but rather by the __mul__ change in the Variable class

Hmm... this should have been catched by the pplpy doctest (https://github.com/sagemath/pplpy/blob/master/ppl/linear_algebra.pyx#L1192)

>>> from ppl import Variable
>>> from gmpy2 import mpz
>>> x = Variable(0)
>>> mpz(3) * x * mpz(5)

@videlec
Copy link
Contributor Author

videlec commented Sep 2, 2023

I indeed misinterpreted https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#arithmetic-methods (which affects __add__ but not __mul__).

@videlec
Copy link
Contributor Author

videlec commented Sep 2, 2023

I indeed misinterpreted https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#arithmetic-methods (which affects __add__ but not __mul__).

This was correctly interpreted, but works only with Cython>=3.0.0 which is not the default version in sage. This explains why doctests pass in conda.

@videlec
Copy link
Contributor Author

videlec commented Sep 3, 2023

Hopefully fixed in sagemath/pplpy#27

@antonio-rojas
Copy link
Contributor

antonio-rojas commented Sep 3, 2023

Can confirm that it fixes all issues in the downstream Arch sagemath package

@github-actions
Copy link

Documentation preview for this PR (built with commit 525dedd; changes) is ready! 🎉

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 1, 2023
sagemathgh-36161: Upgrade pplpy to 0.8.9
    
Upgrade the standard package pplpy to version 0.8.9 (released on PyPI
https://pypi.org/project/pplpy/0.8.9/)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
    
URL: sagemath#36161
Reported by: Vincent Delecroix
Reviewer(s):
@vbraun vbraun merged commit ab6f53b into sagemath:develop Oct 8, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants