WIP: MAINT: Add deprecation warning to views of multi-field indexes #6054

ahaldane · 2015-07-08T16:27:42Z

As discussed in #6053, in preparation for making multi-field indices return a view instead of a copy, this adds a deprecation warning when attempting to view the result of a multi-field index, as in:

>>> a = ones(10, dtype='i8,i8,i8')
>>> b = a[['f0','f2']
>>> b.view('i4') # Deprecation warning

Such code will break any time we convert copies to views, because the byte arrangement of copies and views is necessarily different. I'd like to get it in for 1.10, so we can more easily make the copy->view change in 1.11.

My computer access is limited right now so this PR might be a bit rough, I'll tidy it later. Just posting for feedback now.

One thing I haven't figured out is how to make the unit tests show the deprecation warning: There are unit tests which I know show the warning (test_field_names) but the warnings don't show up when I run np.test('full') or even np.test('full', raise_warnings=[FutureWarning]). I'd like to use this to catch all the numpy code that will break when convertingmulti-field copies to view. Any ideas?

seberg · 2015-07-08T21:20:03Z

Did you run the test on a clean session? Otherwise they are probably suppressed. If you are running them on a clean session, there is a with warnings.catch_warnings missing somewhere in tests that are run earlier :(. Tracking that down is annoying, I am not sure there is a nice way to do it.

The thing to remember with testing warnings is that all warnings must be created with "always" filter active.

EDIT: To be more clear, there may be some other test, which gives the warning, but does not notice there is an extra warning being given. And since the warning is not set to "always", it is ignored afterwards. So there is likely a catch warning context, which does not set the filter for all warnings (the default "once" kills things quickly.

By the way, would it be possible to wrap the whole test suit in an "always" context?

seberg · 2015-07-08T21:35:32Z

Added a PR that might make that problem go away to some degree. (but you always need to use a warning clean session when testing, and that is difficult to provide! E.g. we had it that running numpy tests a second time would give errors because of this kind of issue)

ahaldane · 2015-07-18T17:02:33Z

All right, part of the problem was that the test was being skipped in python2, so it was normal not to see the warning.

If I set raise_warnings in python3, that test does indeed raise, but for a different deprecation that happens before mine. If I run the test manually (by importing it and running it) it correctly shows all the deprecations.

In order to be able to see all the deprecation warnings (and not just the first one in a test) I found I need to also comment out this line, and then run np.test('full'). So that problem is solved too.

I added a test and moved unrelated changes to another PR.

seberg · 2015-07-19T13:18:06Z

I just got a bit worried about multifield subscription/getitem. You know the wonky "buffer" logic in setitem (and we can fix that), but in general I am a bit worried about losing the non-indexed information, i.e. silently overwriting the garbage part of the VOID dtype.
Now the setitem can be fixed, but in general, is it easy to be sure that no garbage will be written into the unused fields which are used in a different view? I do think multifield assignments should certainly work, but just wanted to caution about this before we do anything. It is quite possible that this is really a no issue (after fixing VOID_setitem).

ahaldane · 2015-07-19T16:08:05Z

Just to be clear, this PR should only be merged if we decide to go ahead with multi-field indices returning views (#6053). So really I need to finish that, and we need to decide what the plan is in #6053 before deciding what to do here.

As for avoiding writing garbage to unviewed fields, #6053 is already safe with respect to this. The solution was to prevent use of the 'buffer' logic in VOID_setitem if the dtype has fields, and also in the dtype_transfer path to avoid the "simple copy" case if there are fields (here)

charris · 2016-09-10T18:20:35Z

numpy/core/src/multiarray/convert.c

@@ -601,6 +601,22 @@ PyArray_View(PyArrayObject *self, PyArray_Descr *type, PyTypeObject *pytype)
        subtype = Py_TYPE(self);
    }

+    if (type != NULL && (PyArray_FLAGS(self) & NPY_ARRAY_WARN_ON_WRITE)) {
+        const char *msg =
+            "Numpy has detected that you (may be) taking a view of an array "


Some line breaks would be good.

charris · 2016-09-10T18:24:44Z

@seberg Will your recent work on warnings help make the tests work consistently?

seberg · 2016-09-11T07:48:06Z

Yes, rebase on master, and you can be sure to find all affected tests (unless they already explicitly filter the future warnings or so). That was the point of it mostly.

seberg · 2016-09-11T20:36:29Z

I tried rebasing on master, seems somewhere in a calling function an error check is missing or something, got segfaults when running the test suits (I expected tests failing, but not segfaulting)

ahaldane · 2016-09-12T17:07:38Z

Cool ,the warning improvements sound promising! I'll fix this up tonight and look into the segfault.

charris · 2016-09-19T19:52:54Z

@ahaldane What is the status of this?

ahaldane · 2016-09-20T00:15:29Z

Sorry, I had an unexpected turn of events last week. Updated now.

The segfault is fixed. It was due to a missing check that ret is NULL in array_deepcopy, which is only getting triggered now since a FutureWarning is raised.

I added extra wrappers to all the tests the deprecation will affect. The new warning improvements make it very easy to see what tests need to change! Very nice.

Also, originally I thought we would be making both np.diagonal and multifield-indexing return a view, but it's not so clear for np.diagonal. I've removed reference to diagonal from the warnings for now. Previous discussions about it trailed off inconclusively, see #5407 #5409 and the mailing list discussion linked there.

charris · 2016-09-20T01:32:22Z

Looks like some more tests need ah fixin'.

charris · 2016-09-20T04:54:04Z

Hmm, the remaining error may be fixed by #8066. @gfyoung Thoughts?

gfyoung · 2016-09-20T04:57:03Z

@charris : Not sure how my PR will address this failure. My patch is addressing the DEBUG=1 machine.

charris · 2016-09-20T05:03:34Z

@gfyoung Well, the simple fix for debug introduced the problem with wheels, so I'm thinking the no-cache-dir may have an impact. Guess @seberg should take a look at this also.

gfyoung · 2016-09-20T05:07:52Z

@charris : Hmmm...I guess along the trend of unusual build errors I guess that could be relevant.

seberg · 2016-09-20T05:23:44Z

Oki, seems like you figured out the "ignore" thing. Could also use the new suppress_warnings context, but it does not matter much, unless you want to make sure you only filter the one warning type.

ahaldane · 2016-09-20T05:25:09Z

I think I got the last error: I was ignoring a warning when I should have been recording it. (certain config flags make the numpy warning suite complain about ignored warnings).

seberg · 2016-09-20T15:39:23Z

numpy/core/tests/test_multiarray.py

        assert_(np.can_cast(a.dtype, b.dtype, casting='equiv'))
-        c = a.astype(b.dtype, casting='equiv')
+        with warnings.catch_warnings(record=True) as log:


Just to show it off (I really don't care about it), this could be an alternative:

with suppress_warnings() as sup: sup.filter(FutureWarning)

without checking that there are only FutureWarnings there, or if you want to make sure there is one future warning:

with suppress_warnings() as sup: l = sup.record(FutureWarning) ... assert_(len(l) > 0)

also works.

actually, maybe assert_warns even was a context manager by now, I somewhat think it was....

ahaldane · 2016-09-21T16:58:48Z

updated to use assert_warns, which is much cleaner looking, and added a release note.

charris · 2016-09-23T19:43:58Z

Ready to squash yet?

Behavior of multi-field indexes will change in 1.13: Multi-field indexes will return a view (not a copy) and assignment between structures with non-identical fieldnames occurs "by position" (not "by fieldname"): >>> a = zeros(10, dtype=[('x', 'i8'), ('y', 'i8'), ('z', 'i8')]) >>> a[['x', 'z']].view('i4') # Deprecation warning for multifield view >>> b = ones(10, dtype=[('y', 'i4'), ('x', 'f4')]) >>> a[:] = b # Deprecation warning for multifield assignment

ahaldane · 2016-09-24T21:48:41Z

Squashed.

ahaldane · 2016-09-24T21:49:06Z

Also, tweaked the warning to remove the recommended workaround, to replace arr[['f1', 'f3']].view(newtype) by arr[['f1', 'f3']].copy().view(newtype). That actually wouldn't work.

The actual workaround is to define something like

def pack_fields(arr):
    dt = arr.dtype
    packed_dtype = np.dtype(zip((n,dt.fields[n][0]) for n in dt.names))
    return array(arr, dtype=packed_dtype, copy=True)

then do pack_fields(arr[['f1', 'f3']]).view(newtype), but that seemed too long to put in the warning.

charris · 2016-09-24T23:20:54Z

Hmm, that is a pretty complicated workaround. Maybe we need a packed method or some such. Not sure what the semantics should be for alignment.

charris · 2016-09-24T23:24:39Z

Let's see what complaints we can gin up. Thanks Allan.

ahaldane · 2016-09-26T15:51:55Z

Thanks for the reviews, Chuck and Sebastian!

ahaldane force-pushed the warnview branch from 036c056 to 9cef41f Compare July 18, 2015 16:57

rgommers added component: numpy._core 03 - Maintenance labels Jul 26, 2015

ahaldane force-pushed the warnview branch from 9cef41f to bd8a662 Compare July 28, 2015 16:22

ahaldane mentioned this pull request Aug 7, 2015

MAINT: struct assignment "by field position", multi-field indices return views #6053

Merged

ahaldane mentioned this pull request Jan 21, 2016

Implement structure subset views #5994

Closed

ahaldane force-pushed the warnview branch 2 times, most recently from 9b6f753 to ff79228 Compare June 29, 2016 23:53

charris added this to the 1.12.0 release milestone Jun 30, 2016

ahaldane mentioned this pull request Jul 6, 2016

diag still read-only #7661

Closed

charris reviewed Sep 10, 2016
View reviewed changes

ahaldane force-pushed the warnview branch 2 times, most recently from 4f8e70e to b1cf258 Compare September 20, 2016 00:15

ahaldane force-pushed the warnview branch 2 times, most recently from 528dca7 to 2d62047 Compare September 20, 2016 03:14

ahaldane force-pushed the warnview branch from f486b4a to b4abe18 Compare September 20, 2016 04:11

ahaldane force-pushed the warnview branch from b4abe18 to 3cbcc01 Compare September 20, 2016 05:22

ahaldane force-pushed the warnview branch from 3cbcc01 to 8c861f4 Compare September 20, 2016 14:13

seberg reviewed Sep 20, 2016

View reviewed changes

ahaldane force-pushed the warnview branch from ea82b2a to db07494 Compare September 21, 2016 16:24

ahaldane force-pushed the warnview branch 4 times, most recently from 5eec07a to d8b846f Compare September 21, 2016 17:44

ahaldane force-pushed the warnview branch 3 times, most recently from 6b87001 to 37dd276 Compare September 24, 2016 21:30

ahaldane force-pushed the warnview branch from 37dd276 to de84ba2 Compare September 24, 2016 21:32

charris merged commit a17e905 into numpy:master Sep 24, 2016

This was referenced Sep 24, 2016

BUG: Fixed deepcopy of object arrays. #6456

Closed

ENH: Better error message for invalid axis #7916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: MAINT: Add deprecation warning to views of multi-field indexes #6054

WIP: MAINT: Add deprecation warning to views of multi-field indexes #6054

ahaldane commented Jul 8, 2015

seberg commented Jul 8, 2015

seberg commented Jul 8, 2015

ahaldane commented Jul 18, 2015

seberg commented Jul 19, 2015

ahaldane commented Jul 19, 2015

charris Sep 10, 2016

charris commented Sep 10, 2016

seberg commented Sep 11, 2016

seberg commented Sep 11, 2016

ahaldane commented Sep 12, 2016

charris commented Sep 19, 2016

ahaldane commented Sep 20, 2016

charris commented Sep 20, 2016

charris commented Sep 20, 2016 •

edited

Loading

gfyoung commented Sep 20, 2016

charris commented Sep 20, 2016

gfyoung commented Sep 20, 2016

seberg commented Sep 20, 2016

ahaldane commented Sep 20, 2016

seberg Sep 20, 2016 •

edited

Loading

seberg Sep 20, 2016

ahaldane commented Sep 21, 2016

charris commented Sep 23, 2016

ahaldane commented Sep 24, 2016

ahaldane commented Sep 24, 2016

charris commented Sep 24, 2016

charris commented Sep 24, 2016

ahaldane commented Sep 26, 2016

WIP: MAINT: Add deprecation warning to views of multi-field indexes #6054

WIP: MAINT: Add deprecation warning to views of multi-field indexes #6054

Conversation

ahaldane commented Jul 8, 2015

seberg commented Jul 8, 2015

seberg commented Jul 8, 2015

ahaldane commented Jul 18, 2015

seberg commented Jul 19, 2015

ahaldane commented Jul 19, 2015

charris Sep 10, 2016

Choose a reason for hiding this comment

charris commented Sep 10, 2016

seberg commented Sep 11, 2016

seberg commented Sep 11, 2016

ahaldane commented Sep 12, 2016

charris commented Sep 19, 2016

ahaldane commented Sep 20, 2016

charris commented Sep 20, 2016

charris commented Sep 20, 2016 • edited Loading

gfyoung commented Sep 20, 2016

charris commented Sep 20, 2016

gfyoung commented Sep 20, 2016

seberg commented Sep 20, 2016

ahaldane commented Sep 20, 2016

seberg Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

seberg Sep 20, 2016

Choose a reason for hiding this comment

ahaldane commented Sep 21, 2016

charris commented Sep 23, 2016

ahaldane commented Sep 24, 2016

ahaldane commented Sep 24, 2016

charris commented Sep 24, 2016

charris commented Sep 24, 2016

ahaldane commented Sep 26, 2016

charris commented Sep 20, 2016 •

edited

Loading

seberg Sep 20, 2016 •

edited

Loading