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

Fix Cython 0.28 build break of qhull.pyx #8581

Merged
merged 2 commits into from Mar 24, 2018

Conversation

@oleksandr-pavlyk
Copy link
Contributor

commented Mar 20, 2018

Fixes build break reported in cython/cython#2154

Fixes #8562

@pv

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

@oleksandr-pavlyk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Surprisingly no. Cythonization of modified qhull.pyx goes through for me with Cython 0.27.3.

@ilayn

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

It does fail on Travis though with __doc__ not writable error.

@pv

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

@pv

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

@@ -2353,7 +2362,7 @@ class ConvexHull(_QhullUser):
self._vertices = np.unique(self.simplices)
return self._vertices

ConvexHull.add_points.__func__.__doc__ = _QhullUser._add_points.__doc__
_set_docs(ConvexHull.add_points, _QhullUser._add_points.__doc__)

This comment has been minimized.

Copy link
@pv

pv Mar 21, 2018

Member

The second argument should not have .__doc__.

Delaunay.add_points.__func__.__doc__ = _QhullUser._add_points.__doc__
# Set docstring for foo to docstring of bar, working around change in Cython 0.28
# See https://github.com/scipy/scipy/pull/8581
def _set_docs(foo, bar):

This comment has been minimized.

Copy link
@pv

pv Mar 21, 2018

Member

dst, src, not foo, bar probably.
And maybe _copy_docs.

Fixes cython/cython#2154
while maintaining ability to use Cython 0.27.3 and earlier
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the oleksandr-pavlyk:fix-qhull-pyx branch from a679e3d to 7c4d744 Mar 22, 2018
@oleksandr-pavlyk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2018

All builds were green. I squashed all changes of qhull.pyx into a single commit. Ready to merge.

@rgommers

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

LGTM. This looks like a safe fix - I'd like to take it along for 1.0.1; an incompatibility with latest Cython is annoying. @pv sound okay?

@pv

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

@rgommers rgommers merged commit b00aa14 into scipy:master Mar 24, 2018
4 checks passed
4 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
codecov/project 75.93% (target 1%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgommers

This comment has been minimized.

Copy link
Member

commented Mar 24, 2018

Merged. Thanks @oleksandr-pavlyk, @pv!

@rgommers rgommers added this to the 1.0.1 milestone Mar 24, 2018
@rgommers

This comment has been minimized.

Copy link
Member

commented Mar 24, 2018

backported to 1.0.x in 5d7a2c7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.