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

MAINT: Always use PyCapsule instead of PyCObject in mtrand.pyx #7536

Merged
merged 2 commits into from
Apr 10, 2016

Conversation

charris
Copy link
Member

@charris charris commented Apr 10, 2016

Python 2.7 has a backport of PyCapsule so we no longer need to support PyCObject. This PR makes that change and removes the no longer needed mt_compat.h file.

Some Deprecation warning are also hidden in the numpy/random/tests.

Python 2.7 has a backport of PyCapsule so we no longer need to support
PyCObject. This PR makes that change and removes the no longer needed
mt_compat.h file.
The warning turned up when the numpy/randome/tests were run using

$ python runtests.py -t numpy/random/tests/

It doesn't show when all the tests are run.
@@ -23,6 +23,7 @@

include "Python.pxi"
include "numpy.pxd"
include "cpython/pycapsule.pxd"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious to see if this path is portable to Windows. I suspect not and wonder why the path isn't in the normal cython search path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it isn't a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, windows is totally happy to accept / as a path separator.

@njsmith
Copy link
Member

njsmith commented Apr 10, 2016

Looks fine to me. I'm a little puzzled about why we're using PyCapsule at all here... I guess just because it's easier to have a dict-of-Python-callables than to have a dict-of-cdef-function-pointers? Doesn't matter much I guess.

@charris
Copy link
Member Author

charris commented Apr 10, 2016

dict-of-Python-callables than to have a dict-of-cdef-function-pointers?

Yes, that was the problem, dict-of-cdef-function-pointers didn't work at all.

@charris
Copy link
Member Author

charris commented Apr 10, 2016

I've toyed a bit with making the PyCapsule object public, but can't convince myself that it is a good idea.

@charris
Copy link
Member Author

charris commented Apr 10, 2016

Strictly speaking, should probably check for error from PyCapsule_GetPointer also, but as long as everything is private should be OK to omit that.

@njsmith
Copy link
Member

njsmith commented Apr 10, 2016

Yeah, let's not go adding the whole rk_state machinery to our public abi... We have enough trouble with our abi exposing too much already :-)

@njsmith njsmith merged commit f1b0091 into numpy:master Apr 10, 2016
@charris charris deleted the random-use-pycapsule branch April 10, 2016 21:36
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.

None yet

2 participants