-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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 various user defined type bugs #175
Conversation
For certain types of user defined classes, casting and ufunc loops normally run without the Python API, but occasionally need to throw an error. Currently we assume that !needs_api means no error occur. However, the fastest way to implement such loops is to run without the GIL normally and use PyGILState_Ensure/Release if an error occurs. In order to support this usage pattern, change all post-loop checks from needs_api && PyErr_Occurred() to simply PyErr_Occurred()
Everything here is very minimal, and looks great to me. Just the needs_api flag could use input from more people with an interest in how NumPy should evolve to support multi-threaded parallelism. |
If you wanted to satisfy my use case, the best way would be to provide a numpy routine to grab the GIL from within a numpy thread. However, it isn't terribly important, so I'd pull the rest of the fixes in first and worry about the needs_api stuff later. |
That's a much better approach than what I was thinking, definitely. |
@@ -4475,6 +4475,7 @@ void _strided_masked_wrapper_transfer_function( | |||
/* Cleanup */ | |||
NPY_AUXDATA_FREE(transferdata); | |||
|
|||
/* If needs_api was set to 1, it may have raised a Python exception */ | |||
return (needs_api && PyErr_Occurred()) ? NPY_FAIL : NPY_SUCCEED; | |||
/* Regardless of whether needs_api is 1, we may have raised a Python exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style: multiline comments like so
/*
* blah
* blah
*/
I've got a branch for this ready to merge. I reordered the commits so that the changes involving needs_api follow the others and should be easy to revert if so desired. Is there any reason I shouldn't merge this at this point? |
Let's skip the needs_api change commit, and make a new tracker ticket for the idea Geoffrey suggested. The way his idea would work is numpy supplies a function which is kind of like PyGILState_Ensure, but allows numpy to set up any TLS state necessary to indicate the GIL has been acquired and that it may be necessary to propagate an exception. Once this is implemented, it should be possible to get the best of both worlds working cooperatively. |
I've committed the non needs_api changes. Geoffrey, could you open the suggested ticket and expand on your idea a bit? I'm going to leave this pull request open for a while for discussion. |
Here you go: http://projects.scipy.org/numpy/ticket/2022 |
OK, I'll close this now. |
feat: Add vmovn_[s16|s32|s64|u16|u32|u64]
As per the mailing list discussion, these commits fix various bugs in the support for user defined types.
I will likely put together a minimal test extension module later that demonstrates them, but it may take me several weeks to get around to this.
Thanks,
Geoffrey