Skip to content

MAINT/BUG: Improve error messages for dtype reassigment, fix missing DECREFs #9499

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
Jul 31, 2017
Merged

MAINT/BUG: Improve error messages for dtype reassigment, fix missing DECREFs #9499

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
115 changes: 64 additions & 51 deletions numpy/core/src/multiarray/getset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,8 +438,7 @@ array_descr_set(PyArrayObject *self, PyObject *arg)
{
PyArray_Descr *newtype = NULL;
npy_intp newdim;
int i;
char *msg = "new type not compatible with array.";
int axis;
PyObject *safe;
static PyObject *checkfunc = NULL;

Expand All @@ -461,14 +460,13 @@ array_descr_set(PyArrayObject *self, PyObject *arg)
if (_may_have_objects(PyArray_DESCR(self)) || _may_have_objects(newtype)) {
npy_cache_import("numpy.core._internal", "_view_is_safe", &checkfunc);
if (checkfunc == NULL) {
return -1;
goto fail;
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't previously decref

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch. Have to say I continue to dislike if-statements with side effects... (obviously outside of this PR, though)

}

safe = PyObject_CallFunction(checkfunc, "OO",
PyArray_DESCR(self), newtype);
if (safe == NULL) {
Py_DECREF(newtype);
return -1;
goto fail;
}
Py_DECREF(safe);
}
Expand All @@ -492,58 +490,73 @@ array_descr_set(PyArrayObject *self, PyObject *arg)
}


if ((newtype->elsize != PyArray_DESCR(self)->elsize) &&
(PyArray_NDIM(self) == 0 ||
!PyArray_ISONESEGMENT(self) ||
PyDataType_HASSUBARRAY(newtype))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a lot of unrelated failures in here that justify their own error message. PyArray_ISONESEGMENT becomes the checks for F and C contiguity further down.

goto fail;
}

/* Deprecate not C contiguous and a dimension changes */
if (newtype->elsize != PyArray_DESCR(self)->elsize &&
!PyArray_IS_C_CONTIGUOUS(self)) {
/* 11/27/2015 1.11.0 */
if (DEPRECATE("Changing the shape of non-C contiguous array by\n"
"descriptor assignment is deprecated. To maintain\n"
"the Fortran contiguity of a multidimensional Fortran\n"
"array, use 'a.T.view(...).T' instead") < 0) {
return -1;
/* Changing the size of the dtype results in a shape change */
if (newtype->elsize != PyArray_DESCR(self)->elsize) {
/* forbidden cases */
if (PyArray_NDIM(self) == 0) {
PyErr_SetString(PyExc_ValueError,
"Changing the dtype of a 0d array is only supported "
"if the itemsize is unchanged");
goto fail;
}
}

if (PyArray_IS_C_CONTIGUOUS(self)) {
i = PyArray_NDIM(self) - 1;
}
else {
i = 0;
}
if (newtype->elsize < PyArray_DESCR(self)->elsize) {
/*
* if it is compatible increase the size of the
* dimension at end (or at the front for NPY_ARRAY_F_CONTIGUOUS)
*/
if (PyArray_DESCR(self)->elsize % newtype->elsize != 0) {
else if (PyDataType_HASSUBARRAY(newtype)) {
PyErr_SetString(PyExc_ValueError,
"Changing the dtype to a subarray type is only supported "
"if the total itemsize is unchanged");
goto fail;
}
newdim = PyArray_DESCR(self)->elsize / newtype->elsize;
PyArray_DIMS(self)[i] *= newdim;
PyArray_STRIDES(self)[i] = newtype->elsize;
}
else if (newtype->elsize > PyArray_DESCR(self)->elsize) {
/*
* Determine if last (or first if NPY_ARRAY_F_CONTIGUOUS) dimension
* is compatible
*/
newdim = PyArray_DIMS(self)[i] * PyArray_DESCR(self)->elsize;
if ((newdim % newtype->elsize) != 0) {

/* determine which axis to resize */
if (PyArray_IS_C_CONTIGUOUS(self)) {
axis = PyArray_NDIM(self) - 1;
}
else if (PyArray_IS_F_CONTIGUOUS(self)) {
/* 2015-11-27 1.11.0, gh-6747 */
if (DEPRECATE(
"Changing the shape of an F-contiguous array by "
"descriptor assignment is deprecated. To maintain the "
"Fortran contiguity of a multidimensional Fortran "
"array, use 'a.T.view(...).T' instead") < 0) {
goto fail;
}
axis = 0;
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 means the deprecation warning and the behavior being deprecated are now in the same place.

Copy link
Member

Choose a reason for hiding this comment

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

I've never decided if we should be putting line endings in these error messages. Maybe we should have our own err calls that wrap the messages.

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 think the line endings make things render more messily, especially since the first line gets an arbitrary prefix depending on whether its a warning or error.

Copy link
Member

Choose a reason for hiding this comment

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

That's why I think the error message formatter should take care of it. I don't expect anything in that direction to happen in the next decade...

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, this is not our problem, and is the concern of things higher up in the stack, starting from the python exception-printing mechanism, and extending to a line-wrapping terminal.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I'm happy with my line-wrapping terminal dealing with it at the moment :)

}
else {
/* Don't mention the deprecated F-contiguous support */
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is not particularly helpful - it assumes a future reader knows that in the past these error messages were combined. I'd suggest to remove it or make it clear that this branch is for non-contiguous arrays.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an else clause for if(c-contig) elseif (f-contif) else, so the obvious conclusion is "must be c or f contig". But this would encourage the deprecated use.

PyErr_SetString(PyExc_ValueError,
"To change to a dtype of a different size, the array must "
"be C-contiguous");
goto fail;
}
PyArray_DIMS(self)[i] = newdim / newtype->elsize;
PyArray_STRIDES(self)[i] = newtype->elsize;

if (newtype->elsize < PyArray_DESCR(self)->elsize) {
/* if it is compatible, increase the size of the relevant axis */
if (PyArray_DESCR(self)->elsize % newtype->elsize != 0) {
PyErr_SetString(PyExc_ValueError,
"When changing to a smaller dtype, its size must be a "
"divisor of the size of original dtype");
goto fail;
}
newdim = PyArray_DESCR(self)->elsize / newtype->elsize;
PyArray_DIMS(self)[axis] *= newdim;
PyArray_STRIDES(self)[axis] = newtype->elsize;
}
else if (newtype->elsize > PyArray_DESCR(self)->elsize) {
/* if it is compatible, decrease the size of the relevant axis */
newdim = PyArray_DIMS(self)[axis] * PyArray_DESCR(self)->elsize;
if ((newdim % newtype->elsize) != 0) {
PyErr_SetString(PyExc_ValueError,
"When changing to a larger dtype, its size must be a "
"divisor of the total size in bytes of the last axis "
"of the array.");
goto fail;
}
PyArray_DIMS(self)[axis] = newdim / newtype->elsize;
PyArray_STRIDES(self)[axis] = newtype->elsize;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These two branches are unchanged other than in error message, but are moved within the elsize != elsize check for clarity

}

/* fall through -- adjust type*/
Py_DECREF(PyArray_DESCR(self));
/* Viewing as a subarray increases the number of dimensions */
if (PyDataType_HASSUBARRAY(newtype)) {
/*
* create new array object from data and update
Expand Down Expand Up @@ -573,12 +586,12 @@ array_descr_set(PyArrayObject *self, PyObject *arg)
Py_DECREF(temp);
}

Py_DECREF(PyArray_DESCR(self));
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to move this? It may have already have been overwritten. In any case, it is easier to be sure if it is left where it was.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other hand, leaving it where it was brings into question whether something was looking at a decref'd dtype.

((PyArrayObject_fields *)self)->descr = newtype;
PyArray_UpdateFlags(self, NPY_ARRAY_UPDATE_ALL);
return 0;

fail:
PyErr_SetString(PyExc_ValueError, msg);
Py_DECREF(newtype);
return -1;
}
Expand Down