@eric-wieser Looking forward to your feedback. 😄

Hmm. I have no idea why the CircleCI build failed, I didn't touch Cython. :/

It looks like you built this patch on an out-of-date master from before we had CircleCI working. If you rebase, the CircleCI error will go away.

This is failing because we reparse the arguments for forwarding to __array_ufunc__ - but I don't remember where.

Somewhat tangent comment: How would this fit in with the dream of turning reduce into a gufunc (related to discussions in #8528 #8819)?

Currently gufuncs have a fixed set of common keyword arguments, axis, dtype, casting, extobj, where and so on. Should individual gufuncs be allowed to have additional args besides the default ones, as identity would be?

Or does the fact that reduce will have an extra identity argument mean that it cannot be implemented as a gufunc?

Edit: Related: #8773

Maybe the way to think of it is that a reduce_ufunc(a, out=b) gufunc treats its "out" parameter also as an input. So if you supply an out, that initial value gets read and added on to.

So maybe ultimately, we would have both a reduce-gufunc, and a reduce method. The reduce method's job is to properly set up the out parameter for the reduce-gufunc. The method would have an "identity" argument, while the ufunc itself would read that value from the out arg.

Sorry for the distraction, we can think about this later outside this PR.

The explanation of these changes could be much more complete. And please explain it for all the functions changed.

So, there are two things that worry me here:

• The interface is different to the builtin max(..., default=0), even though it's roughly the same
• ufunc.reduce(identity=...) only really makes sense for maximum. minimum, fmax, and fmin, where choosing a different domain lets you choose a different identity. For other ufuncs, it produces crazy results (I think np.add.reduce(np.ones(2,2), identity=10) will give a surprising result, but I haven't checked).
  • We could add a marker indicating that the identity is configurable by kwarg, alongside the current allowable identity values - and throw an error when the kwarg is passed for a ufunc without that marker
  • For object arrays, having an identity makes sense for lots of ufuncs - consider np.add.reduce(["hello", "world"], dtype=object, identity="")

The explanation of these changes could be much more complete. And please explain it for all the functions changed.

I'll add more complete docs with examples, if that's what you mean, incorporating @eric-wieser's excellent use cases and insights. :-)

The interface is different to the builtin max(..., default=0), even though it's roughly the same.

Yes, I thought of that as well, however, plenty of kwargs here are different (axis, keepdims, etc.). I'll go ahead and assume that we're not following Python's interface here, however, this can still be changed.

However, if it WAS to be changed, I'd say it's more similar to functools.reduce's initializer argument, which is much more instructive. This is also in line with your second worry:

For other ufuncs, it produces crazy results (I think np.add.reduce(np.ones(2,2), identity=10) will give a surprising result, but I haven't checked).

>>> np.add.reduce(np.ones((2, 2)), identity=10)
array([12., 12.])

The name initializer instead of identity is much more instructive and takes care of this worry.

So initializer probably makes more sense here. We could add a warning in the docs that non-default initializers could lead to unexpected results.

will give a surprising result, but I haven't checked

There's a more surprising result than that one to be found, I think. Maybe np.add.reduce(np.ones((2, 2, 2)), axis=(0, 1), identity=10)?

It gives a quite reasonable result, actually.

>>> np.add.reduce(np.ones((2, 2, 2)), axis=(0, 1), identity=10)
array([14., 14.])

The name initializer instead of identity is much more instructive and takes care of this worry.

That's maybe reasonable. Perhaps ufunc.reduce should take an initializer argument, but the np.min shorthand can alias it to default?

We'll definitely need to run this by the list before this gets merged, but may as well brainstorm here first

Non-adjacent axes? axis=(0, 2)?

Non-adjacent axes? (0, 2)?

Still works beautifully.

That's maybe reasonable. Perhaps ufunc.reduce should take an initializer argument, but the np.min shorthand can alias it to default?

The slight problem I see with this is something like the following:

>>> np.minimum.reduce([0.0], identity=-np.inf)
-inf

As you can see, it gives a value not in the original set. I guess we could call it default but then there are two issues with this:

• The one I mentioned above;
• We're deviating from ufunc.reduce norms, which is a bad thing. It doesn't keep uniformity.
• Problems when generalizing to other reductions.

Still works beautifully.

Huh, I think my memory of the code is incorrect. I thought that multi-axis reduce fell back on repeated single-axis reduce, and so was expecting to get 24 as the result. It seems that's not the case, which is great!

As you can see, it gives a value not in the original set.

I'm now on board with the argument being called initializer, which makes that far less confusing.

The issue in that case is that the caller chose to pass in an initializer that was not the minimum of the set of allowed values. That looks perfectly fine to me.

CI is failing because core/src/umath/override.c@normalize_reduce_args is missing this argument.

The issue in that case is that the caller chose to pass in an initializer that was not the minimum of the set of allowed values. That looks perfectly fine to me.

Exactly. We can document a use-case as being reduction of an empty axis.

On that note, I'm going to work and might not be available for the next hours. If you want to fix that on my branch, feel free.

On another note, if we're renaming this to initializer, it would make sense to add it to accumulate and reduceat. Is that best done in another PR or this one?

Edit: I'm willing to take on the task of making reduceat use identity.

reduceat is best untouched - there's an issue about writing a replacement somewhere.

Accumulate would be a better fit for another PR, especially since the use case is less strong.

Edit: I'm willing to take on the task of making reduceat use identity.

#834 should get you started on that rabbit hole. Lets try and wrap up this PR first though!

I think what you, the behaviour for objects is close enough to what the final state should be than what would be the case without those extra commits. So, I would suggest to merge and do a follow-up PR on the details.

+1 for finally merging.

Well, I don't know and I do not want to think about it. Close enough for me is close enough, if assuming that someone will use the current behaviour, we can still get to a reasonable final state without problems at some point. I hate adding the next kludge that future us gets annoyed about.

Right now I am a bit confused what you are suggesting as final state for the object support....

That said, None as the "Error out" value, is maybe good even for object. Not ideal in some sense but the other solutions seem not much nicer. Plus, I am not sure it would even manoeuvre into a tricky to resolve issue for the future.

@seberg Right now, the issue is that if you pass initial, it will always include it in the list even for objects. That could break backwards compatibility, as for some functions in _methods.py, we do pass it. So objects will get 0 in the list when they try to do np.sum, for example.

Right now, having np._NoValue or None as an "ignore me" is what we want.

I think the cause of our problems is how the .identity is defined, and we'd do better to free ourselves from that constraint. What I'd propose then is:

• Use .reduce(initial=np._NoValue) to indicate "no identity"
• Make .reduce(obj_array, initial=some_value) use the "initial" logic, not the "default" logic
• Allow initial=None to be passed like any other value
• Remove the initial=ufunc.identity kwarg defaults, and just remark in the documentation of initial that "If the ufunc has no identity or the dtype is object, this defaults to None - otherwise it defaults to ufunc.identity"

@eric-wieser Would you happen to know how I'd access np._NoValue inside the C code for this check? Doesn't seem terribly hard to do. I'd also need to do the same in override.c.

Something like

#include "npy_import.h"

static PyObject *NoValue = NULL;
npy_cache_import("numpy", "_NoValue", &NoValue);

ought to work

Penultimate commit message is very wrong!

I think this now has the desired behavior for all cases. The np.NoIdentity thing is a matter for a separate PR, this PR keeps the convention that None means "no initial value" and np._NoValue means "ignore initial completely".

Any more changes required here? If not, is this good to merge? cc @eric-wieser @mhvk @seberg

Won't look through the code, at least not now. It seems it still needs release notes I guess? So I understand correctly it is now:

• Initial value for the reduction for all (almost) cases, including object
• None will cause an error (even for object)
• default is NoValue and will cause old behaviour.

Seems good to me as a change.

That's correct:

• initial behaves as an initial value in all cases now, even for dtype=object. Previously, it would behave as the default value for dtype=object and initial value otherwise.
• None means "no initial value, err if empty, otherwise use the start of the array".
• np._NoValue causes the old behavior.

I'll add release notes now.

Rebased and tests added for dtype=object. Tell me when merging is imminent so I can squash all the commits.

@hameerabbasi - I'd gladly merge this (maybe after one more day for last comments), so do go ahead and squash!

@mhvk Ping. 😀

Merged. Thanks, @hameerabbasi!

Thanks so much to @eric-wieser and @mhvk especially for babying me through my first big PR. I owe you!