The change from goto fail to return here and below is because in fail I now need to decref axes (since it might be a new object created from axis). Fortunately, we have not taken any references here (and below), so it is safe to just return.

This is actually not strictly necessary (and checked); signatures like (i),(j)->() are fine for keepdims. Will change after having had further review.

It would be nice to clarify (and test!) it this also works if the output has the same dimension. For example, a signature of (i)->(i).

Yes, indeed. The problem is that the _umath_test module does not contain any example of a gufunc with such a signature, and I don't feel quite up to writing one... (sadly, linalg also does not have one). Note, though, that the code path does get tested - with keepdims=True, any no-core arguments do get upgraded to having a single core dimension.

p.s. Of course this isn't a reason to clarify the documentation! Indeed, the keepdims case itself might be useful for that.

Now added a bit more text to keepdims to clarify this.

Note to myself: missing broadcast and this incorrectly states the single dimension needs to be shared.

A comment with the python type would be nice for this and the above line

You mean something like

# list of tuples of specific axes for each argument
# axis to use for a shared, single core dimension

Am not sure it will help much to have that here.

Yeah, something like that. Maybe even just /* type: T */ and /* type: List[Tuple[T]] */, adopting PEP484 syntax (and considering @shoyer's remarks about non-int types

What if value is np.false_? How do we handle this in subok?

I simply copied the stanza from subok, but agree it is somewhat odd. I'd prefer to leave it for a quick MAINT PR, though.

Any reason not to use a npy_bool for this and out_subok?

In the main routine, it is int, though, and I use -1 as a value to indicate "unitialized". Overall, perhaps best done in separate cleanup.

I would prefer to see the test separate from the error message. Something like bool _ufunc_supports_keepdims(ufunc)

Yes, that makes sense. I'll do the same for axis (even though it is a trivial one).

Why is keepdims being added in the loop? That seems wrong to me

It does need to be there, so that the iterator can allocate dimensions in the correct order (see comment just above this piece of code)

A little misleading for someone greping for in1d and looking for np.in1d

Instead of computing this over and over again, why not something like this at the start:

npy_intp *core_num_dims;
if (keepdims) {
    // or could just use a static buffer
    core_num_dims = alloca(sizeof(*core_num_dims) * NPY_MAX_DIMS);
    for (i = 0; i < nin; ++i) {
         core_num_dims[i] = ufunc->core_num_dims[i];
    }
    for(; i < nop; i++) {
        core_num_dims[i] = 1;
    } 
}
else {
    core_num_dims = ufunc->core_num_dims;
}

OK, better indeed, especially since all dimensions are just 1.

It seems appveyor/windows does not like alloca, so I now just use a static buffer.

I'm starting to think that we should have a type hierarchy for gufuncs, rather than making methods available or not based on their properties. Not something to deal with in this PR, but something to consider.

Yes, that makes sense. For that purpose, the separate _check_..._support functions are also sensible.

This was better with the two ternaries line wrapped to match up

OK, now have all arguments on their own line.

Why not just leave it alone then? This doesn't seem to justify doing either at the moment.

Edit: Looks like the real reason we need it is we allocate a new axes below

I'll edit the comment to state this explicitly.

I think this optimization might make more sense inside normalize_signature_keyword

The reason I put it here is that normalize_signature_keyword gets passed normal_kwds, which by this time may well have gotten an out entry even if there were no original keywords.

Should axis be converted to axes here, so that __array_ufunc__ only needs to support axes?

Perhaps a normalize_axis_keyword(*normal_kwds)

In general, I think __array_ufunc__ should just get whatever is passed in, since in principle at least it may act differently than the normal ufunc. In fact, I am somewhat torn about even including the check here; we do just pass on everything else that the user passes in, even if it would give an error message if no __array_ufunc__ is present. But then, we do check sig and signature. But then again, that is a deprecation...

Isn't the whole point in these normalization function not to pass on whatever was passed? I'd argue that by not normalizing axis, we make it harder for overriders to implement it and axes correctly.

The question really is - is your description of how axis maps to axes an invariant of ufuncs, or just one for the specific case of ndarrays? I think that if a short-hand can mean different things for different subclasses, then it's a dangerous shorthand.

As I saw it, the normalization was mostly to ensure all but the inputs were in keyword arguments, so that __array_ufunc__ implementations didn't have to worry about that; sig and signature seemed to be there just because sig is deprecated.

But I can see your argument as well that one should just translate axis because we should not encourage __array_ufunc__ implementations that can handle axis but not axes. I'll try to see how it looks if I split out the conversion of axis to axes as a helper function, as you suggest below.

I'm finding myself slightly stuck: I now have a nice _build_axes_tuple in ufunc_object.c, but to use this in override.c, I'd need to include it in a .h file -- but in umath, those seem to contain only stuff to be exported, which this obviously is not. Most logical might be to start a new ufunc_parser.c or ufunc_helper.c which contains all these helper functions for parsing, but that feels a bit beyond this PR...

An alternative might be just to remove the override commit for now, with the logic that __array_ufunc__ implementations should do whatever they want; they'll get an error anyway if they pass this back in.

but let's make sure it works for arbitrary Python objects, not just integers.

+1 on this. axis='rows' for a ufunc like (i),(i) -> () should end up as axes=[['rows'], ['rows'], []]

The current normalization does work for arbitrary objects. It just puts axis in a single-element tuple, and creates a list with that tuple as entries (or an empty one if an argument has no core dimensions).

@mhvk: Can you just put it in numpy\core\src\umath\ufunc_object.h? Those functions are not exported either.

Yes, putting it in ufunc_object.h works (I had mistakenly looked in ufuncobject.h, which does have things for export...), but it has a problem that then we need even more parsing... (I'll move to the main thread, since I think this discussion should not disappear after another change.)

Yeah, the fact that both of those filenames exist is super confusing...

nit: type: Tuple[np.dtype] would match the mypy-style annotation used below

done.

nit: inconsistent spacing. Could break this over multiple lines too.

added spaces.

"Not supported for non-generalized ufuncs" might be nice. Does np.add([1], [2], axis=0) fail?

It does say specifically "over which a generalized ufunc should operate. And, yes, an error is raised:

In [2]: np.add(1., 1., axis=0)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-578c404b564c> in <module>()
----> 1 np.add(1., 1., axis=0)

TypeError: 'axis' is an invalid keyword to ufunc 'add'

But this was not tested - now added.

Why is this RuntimeError here but TypeError above (in override.c)? I would expect both to give the same error (probably TypeError).

That was for symmetry with sig and signature, but I must admit a TypeError makes much more sense, so I've changed it.