MAINT: Minor ufunc cleanup #8876
Conversation
| * argument, would have their size set to 1. | ||
| */ | ||
| static int | ||
| _get_coredim_sizes(PyUFuncObject *ufunc, PyArrayObject **op, |
There was a problem hiding this comment.
Almost all of this function, and its doc-comment, are a direct move of the code from before, with the exception that:
goto failis justreturn -1- some fields need re-extracting from ufunc
ufunc_nameis recalculated when needed - only in errors, so performance is irrelevant
| @@ -1983,7 +2107,7 @@ PyUFunc_GeneralizedFunction(PyUFuncObject *ufunc, | |||
| int nin, nout; | |||
| int i, j, idim, nop; | |||
| const char *ufunc_name; | |||
| int retval = -1, subok = 1; | |||
| int retval = 0, subok = 1; | |||
There was a problem hiding this comment.
Starting at 0 makes more sense, because our function works by assigning to it until its not zero
There was a problem hiding this comment.
It's always overwritten, so the value assigned here is irrelevant, right?
There was a problem hiding this comment.
True, but this the correct base case if we end up with a code path that never sets retval, so seems like a good thing to keep
numpy/core/src/umath/ufunc_object.c
Outdated
| ufunc_name, out_op, i, ufunc->core_signature); | ||
| /* Collect the lengths of the labelled core dimensions */ | ||
| retval = _get_coredim_sizes(ufunc, op, core_dim_sizes); | ||
| if(retval < 0) { | ||
| retval = -1; |
There was a problem hiding this comment.
You don't need this assignment anymore, do you?
There was a problem hiding this comment.
I don't follow - why not?
There was a problem hiding this comment.
Oh, wait, yes I do...
| @@ -1983,7 +2107,7 @@ PyUFunc_GeneralizedFunction(PyUFuncObject *ufunc, | |||
| int nin, nout; | |||
| int i, j, idim, nop; | |||
| const char *ufunc_name; | |||
| int retval = -1, subok = 1; | |||
| int retval = 0, subok = 1; | |||
There was a problem hiding this comment.
It's always overwritten, so the value assigned here is irrelevant, right?
|
I see the point, though #8819 will mean that it will need do_remap and remap_axis arguments, which means it will be less stand-alone than it is now. |
|
I don't think that needing to add extra parameters in future is an argument against this function - even if it needs more inputs, it still has one goal, which is to produce the argument passed to the inner loop. Also, when passed |
Previously the ufunc methods would use "(unknown)", but the basic __call__ would use "<unknown ufunc>"
98bbfa8 to
19ae9fb
Compare
|
@jaimefrio: Fixed that stray |
|
Not at all -- I think that one hasn't converged, and I can rebase if need be. My main point was that, with it, the case for splitting this part off became a bit less strong, since now more of the function's state has to be passed on. |
This was more about making it clear which state these loops are producing, that about making it clear what state they take as input |
|
I'd like to pull the trigger on this before we accumulate more technical debt in the ufunc machinery |
|
I'm OK with this being merged; and certainly we should merge the name part. |
|
In it goes then! |
Follows on from numpygh-8876
ufunc->name == NULL