-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
ENH: Unlock the GIL for gufuncs #7198
Conversation
@@ -2343,6 +2344,16 @@ PyUFunc_GeneralizedFunction(PyUFuncObject *ufunc, | |||
} | |||
} | |||
|
|||
total_problem_size = NpyIter_GetIterSize(iter); | |||
if (total_problem_size < 0) { |
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.
Actually I am not sure how reliable this is, but don't have a quick better idea either. With all those inner core dimensions, I guess the iterator size could be vastly overestimated possibly (I am not sure).
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.
Can an ssize_t actually meaningfully overflow here? ssize_t = intp, right? So how do you address an array that's bigger then ssize_t can represent?
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.
Otherwise looks good to me.
.
Obligatory comment in passing that the code duplication between ufuncs and
gufuncs sucks.
On Fri, Feb 5, 2016 at 1:53 PM, seberg notifications@github.com wrote:
In numpy/core/src/umath/ufunc_object.c
#7198 (comment):@@ -2343,6 +2344,16 @@ PyUFunc_GeneralizedFunction(PyUFuncObject *ufunc,
}
}
- total_problem_size = NpyIter_GetIterSize(iter);
- if (total_problem_size < 0) {
Actually I am not sure how reliable this is, but don't have a quick better
idea either. With all those inner core dimensions, I guess the iterator
size could be vastly overestimated possibly (I am not sure).—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/7198/files#r52077858.
Nathaniel J. Smith -- https://vorpus.org http://vorpus.org
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.
The array isn't bigger, just the intermediate representation of the iterator before removing some axes. E.g. a call to numpy.core.umath_tests.inner1d
, which has signature '(i),(i)->()', with two arrays of shapes (a, 1, c)
and (1, b, c)
will create an iterator with shape (a, b, c, c)
before removing the last two dimensions. So with slightly more complicated gufuncs it is a meaningful possibility.
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.
Eh, I guess. Does the iterator actually handle such cases? Should we care when by the time we're dealing with such numbers we're just as likely to roll back around to positive? I guess it doesn't really matter too much whatever we do here...
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.
If it would not care explicitly, it should error out. So it sets it to
-1 and errors out unless you remove that stuff again. I added that
weird feature explicitly for these gufuncs that bloat the iterator.
Happened to notice this while skimming the PR list for something else, and AFAICT from the code and conversation it's good to go. @seberg: could you add a mention in the release notes? Otherwise good to merge I think |
fa97678
to
d6c5e7d
Compare
Added a small note. |
d6c5e7d
to
19655d1
Compare
Thanks @seberg! |
ENH: Unlock the GIL for gufuncs
Frankly, not sure about testing this kind of stuff. I also have a faint feeling that some of the other loops should check
needs_api
on top of the iterators API need.I am sure @juliantaylor likes to comment ;).