8000 MAINT: Removed duplicated code around `ufunc->identity` by eric-wieser · Pull Request #8952 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Removed duplicated code around ufunc->identity #8952

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 18, 2017
Merged

MAINT: Removed duplicated code around ufunc->identity #8952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions numpy/core/src/umath/reduction.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,8 @@ PyArray_InitializeReduceResult(

/*
* This function executes all the standard NumPy reduction function
* boilerplate code, just calling assign_identity and the appropriate
* inner loop function where necessary.
* boilerplate code, just calling the appropriate inner loop function where
* necessary.
*
* operand : The array to be reduced.
* out : NULL, or the array into which to place the result.
Expand All @@ -432,11 +432,11 @@ PyArray_InitializeReduceResult(
* with size one.
* subok : If true, the result uses the subclass of operand, otherwise
* it is always a base class ndarray.
* assign_identity : If NULL, PyArray_InitializeReduceResult is used, otherwise
* this function is called to initialize the result to
* identity : If Py_None, PyArray_InitializeReduceResult is used, otherwise
* this value is used to initialize the result to
* the reduction's unit.
* loop : The loop which does the reduction.
* data : Data which is passed to assign_identity and the inner loop.
* data : Data which is passed to the inner loop.
* buffersize : Buffer size for the iterator. For the default, pass in 0.
* funcname : The name of the reduction function, for error messages.
* errormask : forwarded from _get_bufsize_errmask
Expand All @@ -459,7 +459,7 @@ PyUFunc_ReduceWrapper(PyArrayObject *operand, PyArrayObject *out,
npy_bool *axis_flags, int reorderable,
int keepdims,
int subok,
PyArray_AssignReduceIdentityFunc *assign_identity,
PyObject *identity,
PyArray_ReduceLoopFunc *loop,
void *data, npy_intp buffersize, const char *funcname,
int errormask)
Expand Down Expand Up @@ -500,7 +500,7 @@ PyUFunc_ReduceWrapper(PyArrayObject *operand, PyArrayObject *out,
* Initialize the result to the reduction unit if possible,
* otherwise copy the initial values and get a view to the rest.
*/
if (assign_identity != NULL) {
if (identity != Py_None) {
/*
* If this reduction is non-reorderable, make sure there are
* only 0 or 1 axes in axis_flags.
Expand All @@ -510,7 +510,7 @@ PyUFunc_ReduceWrapper(PyArrayObject *operand, PyArrayObject *out,
goto fail;
}

if (assign_identity(result, data) < 0) {
if (PyArray_FillWithScalar(result, identity) < 0) {
goto fail;
}
op_view = operand;
Expand Down
12 changes: 6 additions & 6 deletions numpy/core/src/umath/reduction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ typedef int (PyArray_ReduceLoopFunc)(NpyIter *iter,

/*
* This function executes all the standard NumPy reduction function
* boilerplate code, just calling assign_identity and the appropriate
* inner loop function where necessary.
* boilerplate code, just calling the appropriate inner loop function where
* necessary.
*
* operand : The array to be reduced.
* out : NULL, or the array into which to place the result.
Expand All @@ -130,11 +130,11 @@ typedef int (PyArray_ReduceLoopFunc)(NpyIter *iter,
* with size one.
* subok : If true, the result uses the subclass of operand, otherwise
* it is always a base class ndarray.
* assign_identity : If NULL, PyArray_InitializeReduceResult is used, otherwise
* this function is called to initialize the result to
* identity : If Py_None, PyArray_InitializeReduceResult is used, otherwise
* this value is used to initialize the result to
* the reduction's unit.
* loop : The loop which does the reduction.
* data : Data which is passed to assign_identity and the inner loop.
* data : Data which is passed to the inner loop.
* buffersize : Buffer size for the iterator. For the default, pass in 0.
* funcname : The name of the reduction function, for error messages.
* errormask : forwarded from _get_bufsize_errmask
Expand All @@ -148,7 +148,7 @@ PyUFunc_ReduceWrapper(PyArrayObject *operand, PyArrayObject *out,
npy_bool *axis_flags, int reorderable,
int keepdims,
int subok,
PyArray_AssignReduceIdentityFunc *assign_identity,
PyObject *identity,
PyArray_ReduceLoopFunc *loop,
void *data, npy_intp buffersize, const char *funcname,
int errormask);
Expand Down
191 changes: 74 additions & 117 deletions numpy/core/src/umath/ufunc_object.c
View file
Open in desktop
67E6
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,6 @@
static int
_does_loop_use_arrays(void *data);

static int
assign_reduce_identity_zero(PyArrayObject *result, void *data);

static int
assign_reduce_identity_minusone(PyArrayObject *result, void *data);

static int
assign_reduce_identity_one(PyArrayObject *result, void *data);


/*UFUNC_API*/
NPY_NO_EXPORT int
PyUFunc_getfperr(void)
Expand Down Expand Up @@ -1847,6 +1837,42 @@ _get_coredim_sizes(PyUFuncObject *ufunc, PyArrayObject **op,
return 0;
}

/*
* Returns a new reference
* TODO: store a reference in the ufunc object itself, rather than
* constructing one each time
*/
static PyObject *
_get_identity(PyUFuncObject *ufunc, npy_bool *reorderable) {
switch(ufunc->identity) {
case PyUFunc_One:
*reorderable = 1;
return PyInt_FromLong(1);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be True when used internally, and 1 when viewed externally.

The upshot is that this fixes #8860.

Ideally, we'd use True for functions like logical_or, and 1 for functions like add


case PyUFunc_Zero:
*reorderable = 1;
return PyInt_FromLong(0);

case PyUFunc_MinusOne:
*reorderable = 1;
return PyInt_FromLong(-1);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we cached these values in a static variable. Is this something we should keep doing?


case PyUFunc_ReorderableNone:
*reorderable = 1;
Py_RETURN_NONE;

case PyUFunc_None:
*reorderable = 0;
Py_RETURN_NONE;

default:
PyErr_Format(PyExc_ValueError,
"ufunc %s has an invalid identity", ufunc_get_name_cstr(ufunc));
return NULL;
}
}


static int
PyUFunc_GeneralizedFunction(PyUFuncObject *ufunc,
PyObject *args, PyObject *kwds,
Expand Down Expand Up @@ -2249,34 +2275,27 @@ PyUFunc_GeneralizedFunction(PyUFuncObject *ufunc,
* product of two zero-length arrays will be a scalar,
* which has size one.
*/
npy_bool reorderable;
PyObject *identity = _get_identity(ufunc, &reorderable);
if (identity == NULL) {
retval = -1;
goto fail;
}

for (i = nin; i < nop; ++i) {
if (PyArray_SIZE(op[i]) != 0) {
switch (ufunc->identity) {
case PyUFunc_Zero:
assign_reduce_identity_zero(op[i], NULL);
break;
case PyUFunc_One:
assign_reduce_identity_one(op[i], NULL);
break;
case PyUFunc_MinusOne:
assign_reduce_identity_minusone(op[i], NULL);
break;
case PyUFunc_None:
case PyUFunc_ReorderableNone:
PyErr_Format(PyExc_ValueError,
"ufunc %s ",
ufunc_name);
retval = -1;
goto fail;
default:
PyErr_Format(PyExc_ValueError,
"ufunc %s has an invalid identity for reduction",
ufunc_name);
retval = -1;
goto fail;
if (identity == Py_None) {
PyErr_Format(PyExc_ValueError,
"ufunc %s ",
ufunc_name);
Py_DECREF(identity);
retval = -1;
goto fail;
}
PyArray_FillWithScalar(op[i], identity);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assign_identity was never anything but this operation

}
}
Py_DECREF(identity);
}

/* Check whether any errors occurred during the loop */
Expand Down Expand Up @@ -2665,31 +2684,6 @@ reduce_type_resolver(PyUFuncObject *ufunc, PyArrayObject *arr,
return 0;
}

static int
assign_reduce_identity_zero(PyArrayObject *result, void *NPY_UNUSED(data))
{
return PyArray_FillWithScalar(result, PyArrayScalar_False);
}

static int
assign_reduce_identity_one(PyArrayObject *result, void *NPY_UNUSED(data))
{
return PyArray_FillWithScalar(result, PyArrayScalar_True);
}

static int
assign_reduce_identity_minusone(PyArrayObject *result, void *NPY_UNUSED(data))
{
static PyObject *MinusOne = NULL;

if (MinusOne == NULL) {
if ((MinusOne = PyInt_FromLong(-1)) == NULL) {
return -1;
}
}
return PyArray_FillWithScalar(result, MinusOne);
}

static int
reduce_loop(NpyIter *iter, char **dataptrs, npy_intp *strides,
npy_intp *countptr, NpyIter_IterNextFunc *iternext,
Expand Down Expand Up @@ -2795,11 +2789,12 @@ static PyArrayObject *
PyUFunc_Reduce(PyUFuncObject *ufunc, PyArrayObject *arr, PyArrayObject *out,
int naxes, int *axes, PyArray_Descr *odtype, int keepdims)
{
int iaxes, reorderable, ndim;
int iaxes, ndim;
npy_bool reorderable;
npy_bool axis_flags[NPY_MAXDIMS];
PyArray_Descr *dtype;
PyArrayObject *result;
PyArray_AssignReduceIdentityFunc *assign_identity = NULL;
PyObject *identity = NULL;
const char *ufunc_name = ufunc_get_name_cstr(ufunc);
/* These parameters come from a TLS global */
int buffersize = 0, errormask = 0;
Expand All @@ -2820,72 +2815,41 @@ PyUFunc_Reduce(PyUFuncObject *ufunc, PyArrayObject *arr, PyArrayObject *out,
axis_flags[axis] = 1;
}

switch (ufunc->identity) {
case PyUFunc_Zero:
assign_identity = &assign_reduce_identity_zero;
reorderable = 1;
/*
* The identity for a dynamic dtype like
* object arrays can't be used in general
*/
if (PyArray_ISOBJECT(arr) && PyArray_SIZE(arr) != 0) {
assign_identity = NULL;
}
break;
case PyUFunc_One:
assign_identity = &assign_reduce_identity_one;
reorderable = 1;
/*
* The identity for a dynamic dtype like
* object arrays can't be used in general
*/
if (PyArray_ISOBJECT(arr) && PyArray_SIZE(arr) != 0) {
assign_identity = NULL;
}
break;
case PyUFunc_MinusOne:
assign_identity = &assign_reduce_identity_minusone;
reorderable = 1;
/*
* The identity for a dynamic dtype like
* object arrays can't be used in general
*/
if (PyArray_ISOBJECT(arr) && PyArray_SIZE(arr) != 0) {
assign_identity = NULL;
}
break;

case PyUFunc_None:
reorderable = 0;
break;
case PyUFunc_ReorderableNone:
reorderable = 1;
break;
default:
PyErr_Format(PyExc_ValueError,
"ufunc %s has an invalid identity for reduction",
ufunc_name);
return NULL;
if (_get_bufsize_errmask(NULL, "reduce", &buffersize, &errormask) < 0) {
Copy link
Member Author
@eric-wieser eric-wieser Apr 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this check to occur first makes cleanup a little easier.

return NULL;
}

if (_get_bufsize_errmask(NULL, "reduce", &buffersize, &errormask) < 0) {
/* Get the identity */
identity = _get_identity(ufunc, &reorderable);
if (identity == NULL) {
return NULL;
}
/*
* The identity for a dynamic dtype like
* object arrays can't be used in general
*/
if (identity != Py_None && PyArray_ISOBJECT(arr) && PyArray_SIZE(arr) != 0) {
Py_DECREF(identity);
identity = Py_None;
Py_INCREF(identity);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this behaviour is justified, but this is a maintenance commit, so for now, we should leave it in


/* Get the reduction dtype */
if (reduce_type_resolver(ufunc, arr, odtype, &dtype) < 0) {
Py_DECREF(identity);
return NULL;
}

result = PyUFunc_ReduceWrapper(arr, out, NULL, dtype, dtype,
NPY_UNSAFE_CASTING,
axis_flags, reorderable,
keepdims, 0,
assign_identity,
identity,
reduce_loop,
ufunc, buffersize, ufunc_name, errormask);

Py_DECREF(dtype);
Py_DECREF(identity);
return result;
}

Expand Down Expand Up @@ -5377,15 +5341,8 @@ ufunc_get_name(PyUFuncObject *ufunc)
static PyObject *
ufunc_get_identity(PyUFuncObject *ufunc)
{
switch(ufunc->identity) {
case PyUFunc_One:
return PyInt_FromLong(1);
case PyUFunc_Zero:
return PyInt_FromLong(0);
case PyUFunc_MinusOne:
return PyInt_FromLong(-1);
}
Py_RETURN_NONE;
npy_bool reorderable;
return _get_identity(ufunc, &reorderable);
}

static PyObject *
Expand Down
0