MAINT: Address many of Marten's comments

6 files changed, 39 insertions, 20 deletions

diff --git a/numpy/core/include/numpy/experimental_dtype_api.h b/numpy/core/include/numpy/experimental_dtype_api.h
index 10a9b4659..10015634d 100644
--- a/numpy/core/include/numpy/experimental_dtype_api.h
+++ b/numpy/core/include/numpy/experimental_dtype_api.h
@@ -331,7 +331,6 @@ typedef int (PyArrayMethod_StridedLoop)(PyArrayMethod_Context *context,
  * Query an ArrayMethod for the initial value for use in reduction.
  *
  * @param context The arraymethod context, mainly to access the descriptors.
- * @param initial Pointer to initial data to be filled (if possible)
  * @param reduction_is_empty Whether the reduction is empty, when it is the
  *     default value is required, otherwise an identity value to start the
  *     the reduction.  These might differ, examples:
@@ -340,6 +339,7 @@ typedef int (PyArrayMethod_StridedLoop)(PyArrayMethod_Context *context,
  *     - We use no identity for object, but `0` and `1` for sum and prod.
  *     - `-inf` or `INT_MIN` for `max` is an identity, but at least `INT_MIN`
  *       not a good *default* when there are no items.
+ * @param initial Pointer to initial data to be filled (if possible)
  *
  * @returns -1, 0, or 1 indicating error, no initial value, and initial being
  *     successfully filled.  Errors must not be given where 0 is correct, NumPy
@@ -347,9 +347,8 @@ typedef int (PyArrayMethod_StridedLoop)(PyArrayMethod_Context *context,
  */
  #define NPY_METH_get_reduction_initial 4
 typedef int (get_reduction_intial_function)(
-        PyArrayMethod_Context *context, char *initial,
-        npy_bool reduction_is_empty);
-
+        PyArrayMethod_Context *context, npy_bool reduction_is_empty,
+        char *initial);
 
 /*
  * ****************************
diff --git a/numpy/core/src/multiarray/array_method.h b/numpy/core/src/multiarray/array_method.h
index ab27cdc62..0aca26d3b 100644
--- a/numpy/core/src/multiarray/array_method.h
+++ b/numpy/core/src/multiarray/array_method.h
@@ -108,7 +108,6 @@ typedef int (get_loop_function)(
  * Query an ArrayMethod for the initial value for use in reduction.
  *
  * @param context The arraymethod context, mainly to access the descriptors.
- * @param initial Pointer to initial data to be filled (if possible)
  * @param reduction_is_empty Whether the reduction is empty, when it is the
  *     default value is required, otherwise an identity value to start the
  *     the reduction.  These might differ, examples:
@@ -117,14 +116,15 @@ typedef int (get_loop_function)(
  *     - We use no identity for object, but `0` and `1` for sum and prod.
  *     - `-inf` or `INT_MIN` for `max` is an identity, but at least `INT_MIN`
  *       not a good *default* when there are no items.
+ * @param initial Pointer to initial data to be filled (if possible)
  *
  * @returns -1, 0, or 1 indicating error, no initial value, and initial being
  *     successfully filled.  Errors must not be given where 0 is correct, NumPy
  *     may call this even when not strictly necessary.
  */
 typedef int (get_reduction_intial_function)(
-        PyArrayMethod_Context *context, char *initial,
-        npy_bool reduction_is_empty);
+        PyArrayMethod_Context *context, npy_bool reduction_is_empty,
+        char *initial);
 
 /*
  * The following functions are only used be the wrapping array method defined
diff --git a/numpy/core/src/umath/legacy_array_method.c b/numpy/core/src/umath/legacy_array_method.c
index 88e55fcb2..81c7585d6 100644
--- a/numpy/core/src/umath/legacy_array_method.c
+++ b/numpy/core/src/umath/legacy_array_method.c
@@ -243,8 +243,8 @@ get_wrapped_legacy_ufunc_loop(PyArrayMethod_Context *context,
  */
 static int
 copy_cached_initial(
-        PyArrayMethod_Context *context, char *initial,
-        npy_bool NPY_UNUSED(reduction_is_empty))
+        PyArrayMethod_Context *context, npy_bool NPY_UNUSED(reduction_is_empty),
+        char *initial)
 {
     memcpy(initial, context->method->initial, context->descriptors[0]->elsize);
     return 1;
@@ -253,12 +253,17 @@ copy_cached_initial(
 
 /*
  * The default `get_reduction_initial` attempts to look up the identity
- * from the calling ufunc.
+ * from the calling ufunc.  This might fail, so we only call it when necessary.
+ *
+ * For our numbers, we can easily cache it, so do so after the first call
+ * by overriding the function with `copy_cache_initial`.  This path is not
+ * publically available.  That could be added, and for a custom initial getter
+ * it will usually be static/compile time data anyway.
  */
 static int
 get_initial_from_ufunc(
-        PyArrayMethod_Context *context, char *initial,
-        npy_bool reduction_is_empty)
+        PyArrayMethod_Context *context, npy_bool reduction_is_empty,
+        char *initial)
 {
     if (context->caller == NULL
             || !PyObject_TypeCheck(context->caller, &PyUFunc_Type)) {
@@ -375,7 +380,6 @@ PyArray_NewLegacyWrappingArrayMethod(PyUFuncObject *ufunc,
             flags |= NPY_METH_IS_REORDERABLE;
         }
         if (identity_obj != Py_None) {
-            /* NOTE: We defer, just in case it fails in weird cases: */
             get_reduction_intial = &get_initial_from_ufunc;
         }
     }
diff --git a/numpy/core/src/umath/reduction.c b/numpy/core/src/umath/reduction.c
index caf4e6fa4..23127024b 100644
--- a/numpy/core/src/umath/reduction.c
+++ b/numpy/core/src/umath/reduction.c
@@ -320,7 +320,7 @@ PyUFunc_ReduceWrapper(PyArrayMethod_Context *context,
              * we will not need the identity as the result is also empty.
              */
             int has_initial = context->method->get_reduction_initial(
-                    context, initial_buf, empty_iteration);
+                    context, empty_iteration, initial_buf);
             if (has_initial < 0) {
                 goto fail;
             }
diff --git a/numpy/core/src/umath/wrapping_array_method.c b/numpy/core/src/umath/wrapping_array_method.c
index 2a8ae14bf..c32b5c714 100644
--- a/numpy/core/src/umath/wrapping_array_method.c
+++ b/numpy/core/src/umath/wrapping_array_method.c
@@ -185,8 +185,8 @@ wrapping_method_get_loop(
  */
 static int
 wrapping_method_get_identity_function(
-        PyArrayMethod_Context *context, char *item,
-        npy_bool reduction_is_empty)
+        PyArrayMethod_Context *context, npy_bool reduction_is_empty,
+        char *item)
 {
     /* Copy the context, and replace descriptors: */
     PyArrayMethod_Context orig_context = *context;
@@ -202,7 +202,7 @@ wrapping_method_get_identity_function(
         return -1;
     }
     int res = context->method->wrapped_meth->get_reduction_initial(
-            &orig_context, item, reduction_is_empty);
+            &orig_context, reduction_is_empty, item);
     for (int i = 0; i < nin + nout; i++) {
         Py_DECREF(orig_descrs);
     }
diff --git a/numpy/core/tests/test_ufunc.py b/numpy/core/tests/test_ufunc.py
index 097edb446..55122d697 100644
--- a/numpy/core/tests/test_ufunc.py
+++ b/numpy/core/tests/test_ufunc.py
@@ -1658,9 +1658,10 @@ class TestUfunc:
         # For an object loop, the default value 0 with type int is used:
         assert type(np.add.reduce([], dtype=object)) is int
         out = np.array(None, dtype=object)
-        # When the loop is float but `out` is object this does not happen:
+        # When the loop is float64 but `out` is object this does not happen,
+        # the result is float64 cast to object (which gives Python `float`).
         np.add.reduce([], out=out, dtype=np.float64)
-        assert type(out[()]) is not int
+        assert type(out[()]) is float
 
     def test_initial_reduction(self):
         # np.minimum.reduce is an identityless reduction
@@ -1701,11 +1702,16 @@ class TestUfunc:
         # Not OK, the reduction itself is empty and we have no idenity
         with pytest.raises(ValueError):
             np.true_divide.reduce(arr, axis=0)
+
         # Test that an empty reduction fails also if the result is empty
         arr = np.zeros((0, 0, 5))
         with pytest.raises(ValueError):
             np.true_divide.reduce(arr, axis=1)
 
+        # Division reduction makes sense with `initial=1` (empty or not):
+        res = np.true_divide.reduce(arr, axis=1, initial=1)
+        assert_array_equal(res, np.ones((0, 5)))
+
     @pytest.mark.parametrize('axis', (0, 1, None))
     @pytest.mark.parametrize('where', (np.array([False, True, True]),
                                        np.array([[True], [False], [True]]),
@@ -2644,7 +2650,8 @@ def test_reduce_casterrors(offset):
     with pytest.raises(ValueError, match="invalid literal"):
         # This is an unsafe cast, but we currently always allow that.
         # Note that the double loop is picked, but the cast fails.
-        # `initial=None` disables the use of an identity here.
+        # `initial=None` disables the use of an identity here to test failures
+        # while copying the first values path (not used when identity exists).
         np.add.reduce(arr, dtype=np.intp, out=out, initial=None)
     assert count == sys.getrefcount(value)
     # If an error occurred during casting, the operation is done at most until
@@ -2654,6 +2661,15 @@ def test_reduce_casterrors(offset):
     assert out[()] < value * offset
 
 
+def test_object_reduce_cleanup_on_failure():
+    # Test cleanup, including of the initial value (manually provided or not)
+    with pytest.raises(TypeError):
+        np.add.reduce([1, 2, None], initial=4)
+
+    with pytest.raises(TypeError):
+        np.add.reduce([1, 2, None])
+
+
 @pytest.mark.skipif(IS_WASM, reason="fp errors don't work in wasm")
 @pytest.mark.parametrize("method",
         [np.add.accumulate, np.add.reduce,