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

eric-wieser · 2017-07-31T09:03:29Z

No description provided.

eric-wieser · 2017-07-31T09:04:02Z

numpy/core/src/multiarray/getset.c

@@ -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;


Didn't previously decref

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

eric-wieser · 2017-07-31T09:04:57Z

numpy/core/src/multiarray/getset.c

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


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.

eric-wieser · 2017-07-31T09:21:12Z

numpy/core/src/multiarray/getset.c

+                        "array, use 'a.T.view(...).T' instead") < 0) {
+                goto fail;
+            }
+            axis = 0;


This means the deprecation warning and the behavior being deprecated are now in the same place.

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.

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.

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...

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.

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

eric-wieser · 2017-07-31T09:26:24Z

numpy/core/src/multiarray/getset.c

+            }
+            PyArray_DIMS(self)[axis] = newdim / newtype->elsize;
+            PyArray_STRIDES(self)[axis] = newtype->elsize;
+        }


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

eric-wieser · 2017-07-31T09:27:32Z

numpy/core/src/multiarray/getset.c

+        else if (PyArray_IS_F_CONTIGUOUS(self)) {
+            /* 2015-11-27 1.11.0 */
+            if (DEPRECATE(
+                        "Changing the shape of an F-contiguous array by "


"non-C contiguous" → "F-contiguous". All other stride layouts were already caught by PyArray_ISONESEGMENT anyway

Isn't PyArray_ISONESEGMENT gone?

Also wondered that, or is it just caught by the else clause below?

PyArray_ISONESEGMENT == PyArray_IS_F_CONTIGUOUS | PyArray_IS_C_CONTIGUOUS | ZERO_DIM, so this is covered by that else clause

Also, as far as I can tell, zero dimensional always implies F_CONTIGUOUS && C_CONTIGUOUS

charris · 2017-07-31T13:18:33Z

numpy/core/src/multiarray/getset.c

+            if ((newdim % newtype->elsize) != 0) {
+                PyErr_SetString(PyExc_ValueError,
+                        "When changing to a larger dtype, its size must be a "
+                        "multiple of the size of original dtype");


Not quite accurate, what this looks like is that the new size must divide the total size of the axis.

Almost seems too loose, although already the case. Could equally apply to the < case. Seems like it should also be a multiple of the original type.

Good catch, I'll update the message.

I don't think we can make it less loose without a deprecation cycle, and might be in favor of making the other case more loose (as in #6097). Having said that, I can't think of a single reason this would be useful)

charris · 2017-07-31T13:59:02Z

numpy/core/src/multiarray/getset.c

@@ -573,12 +583,12 @@ array_descr_set(PyArrayObject *self, PyObject *arg)
        Py_DECREF(temp);
    }

+    Py_DECREF(PyArray_DESCR(self));


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.

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

charris · 2017-07-31T14:00:47Z

Couple of guestions, generally looks good.

mhvk

Great to see the previous somewhat illogical behaviour of view adjusted!

mhvk · 2017-07-31T16:07:02Z

numpy/core/src/multiarray/getset.c

@@ -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;


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

mhvk · 2017-07-31T16:11:02Z

numpy/core/src/multiarray/getset.c

+        else if (PyArray_IS_F_CONTIGUOUS(self)) {
+            /* 2015-11-27 1.11.0 */
+            if (DEPRECATE(
+                        "Changing the shape of an F-contiguous array by "


Also wondered that, or is it just caught by the else clause below?

mhvk · 2017-07-31T16:12:20Z

numpy/core/src/multiarray/getset.c

+            axis = 0;
+        }
+        else {
+            /* Don't mention the deprecated F-contiguous support */


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.

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.

eric-wieser · 2017-07-31T18:47:04Z

behaviour of view adjusted!

As a MAINT PR, there is no (deliberate) behaviour change here - what change did you think this made?

…DECREF

eric-wieser · 2017-07-31T19:03:14Z

Updated with a fix to the error message noticed by @charris, and a slightly clearer message for the non-contiguous case

charris · 2017-07-31T19:52:19Z

Thanks Eric.

eric-wieser · 2017-07-31T20:29:05Z

Thanks for the fast merge - I'll rebase and reopen my other PR now

eric-wieser added 00 - Bug 03 - Maintenance labels Jul 31, 2017

eric-wieser commented Jul 31, 2017

View reviewed changes

eric-wieser mentioned this pull request Jul 31, 2017

BUG/MAINT/ENH: Allow viewing where only the last axis is contiguous #9497

Closed

charris reviewed Jul 31, 2017

View reviewed changes

mhvk approved these changes Jul 31, 2017

View reviewed changes

MAINT/BUG: Improve error messages for dtype reassigment, fix missing …

8cc4480

…DECREF

eric-wieser force-pushed the fix-view-decrefs branch from fdd3982 to 8cc4480 Compare July 31, 2017 19:02

charris merged commit be7266d into numpy:master Jul 31, 2017

eric-wieser mentioned this pull request Jul 31, 2017

MAINT: Move variables into deepest relevant scope, for clarity #9503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eric-wieser commented Jul 31, 2017

eric-wieser Jul 31, 2017

mhvk Jul 31, 2017

eric-wieser Jul 31, 2017

eric-wieser Jul 31, 2017

charris Jul 31, 2017

eric-wieser Jul 31, 2017

charris Jul 31, 2017

eric-wieser Jul 31, 2017

charris Jul 31, 2017

eric-wieser Jul 31, 2017

eric-wieser Jul 31, 2017

eric-wieser Jul 31, 2017

charris Jul 31, 2017

mhvk Jul 31, 2017

eric-wieser Jul 31, 2017 •

edited

Loading

eric-wieser Jul 31, 2017

charris Jul 31, 2017

charris Jul 31, 2017

eric-wieser Jul 31, 2017

charris Jul 31, 2017

eric-wieser Jul 31, 2017

charris commented Jul 31, 2017

mhvk left a comment

mhvk Jul 31, 2017

mhvk Jul 31, 2017

mhvk Jul 31, 2017

eric-wieser Jul 31, 2017

eric-wieser commented Jul 31, 2017 •

edited

Loading

eric-wieser commented Jul 31, 2017

charris commented Jul 31, 2017

eric-wieser commented Jul 31, 2017

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

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

Conversation

eric-wieser commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Jul 31, 2017

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Jul 31, 2017 • edited Loading

eric-wieser commented Jul 31, 2017

charris commented Jul 31, 2017

eric-wieser commented Jul 31, 2017

eric-wieser Jul 31, 2017 •

edited

Loading

eric-wieser commented Jul 31, 2017 •

edited

Loading