MAINT: Warn if out argument has refcnt 1 and owns its data. #9632

MSeifert04 · 2017-08-31T01:26:39Z

Based on the feedback in #7244 but it doesn't actually solve it (see below) emit a RuntimeWarning in case the out argument seems dubious. Note that this test can lead to false positives in case someone actually catches the result:

res = np.add([1,2], [3,4], out=np.empty(2))

And it won't trigger if someone actually makes a new array and creates a view on that:

res = np.add([1,2], [3,4], out=np.empty(2).ravel())

MSeifert04 · 2017-08-31T11:12:18Z

numpy/core/tests/test_ufunc.py

+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            np.add(arr, arr, out=np.empty(3))
+        assert len(w) == 1


Probably needs to be skipped on non-refcounting Pythons (PyPy).

You can also just use assert_warns (or also suppress_warnings since you also match the text). I just like it a bit more:

with suppress_warnings() as sup: r = sup.record(RutimeWarning, ".*the result may be lost") ... assert_(len(r) == 1)

but I do not care much about it.

And yes, you have to check for refcounting of the interpreter (I am not sure if the C-Code needs something here).

Do we allow assert statements nowadays? I something think we might, but we used to not use them (since they are more commonly used with pytest?)

MSeifert04 · 2017-08-31T11:15:18Z

and no idea why this fails on Python 3.6. Did they change something in warnings.catch_warnings?

Also if that change would be too disruptive I won't mind if it's declined.

seberg

Not sure why it fails in 3.6, there were some minor changes, but I do not really see them affecting the tests/catch_warnings in any case.

I think I like the change as such, there is practically no use case for this case, so it probably almost always will be a programming oversight, in that sense, I should think a bit about what the warning should say.

The PyPy stuff needs to be fixed (I am not sure what Py_REFNT does on PyPy, but it might be best to just put the whole thing into an #ifndef block.

seberg · 2017-09-06T16:59:53Z

numpy/core/tests/test_mem_overlap.py

@@ -887,7 +892,9 @@ def test_unary_ufunc_where_same(self):
        ufunc = np.invert

        def check(a, out, mask):
-            c1 = ufunc(a, out=out.copy(), where=mask.copy())
+            with warnings.catch_warnings(record=True) as w:
+                warnings.simplefilter("always")


Please use:

with suppress_warnings() as sup: sup.filter(WarningType, [warning_message_regexp])

its not a big thing, but its nicer since it is more specific (especially if you put the message text).

seberg · 2017-09-06T17:02:07Z

numpy/core/tests/test_ufunc.py

+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            np.add(arr, arr, out=np.empty(3))
+        assert len(w) == 1


You can also just use assert_warns (or also suppress_warnings since you also match the text). I just like it a bit more:

with suppress_warnings() as sup: r = sup.record(RutimeWarning, ".*the result may be lost") ... assert_(len(r) == 1)

but I do not care much about it.

seberg · 2017-09-06T17:05:04Z

numpy/core/tests/test_ufunc.py

+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            np.add(arr, arr, out=np.empty(3))
+        assert len(w) == 1


And yes, you have to check for refcounting of the interpreter (I am not sure if the C-Code needs something here).

seberg · 2017-09-06T17:05:53Z

numpy/core/tests/test_ufunc.py

+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            np.add(arr, arr, out=np.empty(3))
+        assert len(w) == 1


Do we allow assert statements nowadays? I something think we might, but we used to not use them (since they are more commonly used with pytest?)

MSeifert04 · 2017-09-06T20:42:13Z

Interesting. It really doesn't warn on Python3.6. I'm not sure what the reason for this is though. Probably the out argument has a refcount of more than 1...

MSeifert04 · 2017-09-06T20:43:15Z

@seberg Thanks for all the suggestions. I should've investigated how NumPy does these warning tests before submitting the PR.

MSeifert04 · 2017-09-06T21:20:57Z

okay, some investigation later ... it's really that a simple example like this:

>>> import numpy as np
>>> np.add([1,2,3], [1,2,3], out=np.empty(3))
array([ 2.,  4.,  6.])

doesn't warn on Python 3.6 because the reference count for out is 2. Strangely it seems like Python 3.6 somewhere adds one reference. Even in the normal case

>>> arr = np.empty(3)
>>> np.add([1,2,3], [1,2,3], out=arr)

The out argument has a reference count of 3 there.

njsmith · 2017-09-06T21:55:29Z

@MSeifart04: Probably there's one reference for the value on the stack, and one for the value passed into numpy, or something like that.

I wonder if this means that @juliantaylor's temporary elision is broken on 3.6...

charris · 2017-09-06T23:04:49Z

I think elision require big arrays. I don't see that here

In [14]: arr = np.empty(3)

In [15]: sys.getrefcount(arr)
Out[15]: 2

In [16]: for i in range(10):
    ...:     np.add([1,2,3], [1,2,3], out=arr)
    ...:     

In [17]: sys.getrefcount(arr)
Out[17]: 2

EDIT: Python 3.6.2

MSeifert04 · 2017-09-06T23:27:17Z

@charris

I tested it with this change:

@@ -501,10 +501,11 @@ _set_out_array(PyObject *obj, PyArrayObject **store)
         /* Translate None to NULL */
         return 0;
     }
     if PyArray_Check(obj) {
 #ifndef PYPY_VERSION
+        printf("refcount: %i\n", (int)Py_REFCNT(obj));
         if (Py_REFCNT(obj) == 1 &&
                 (PyArray_FLAGS((PyArrayObject *)obj) & NPY_ARRAY_OWNDATA)) {
             if (PyErr_WarnEx(PyExc_RuntimeWarning,
                              "the output array seems to be a new array, if the "
                              "returned values of the ufunc are not assigned to "

with these results:

Python 3.6.2 |Continuum Analytics, Inc.| (default, Jul 20 2017, 12:30:02) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> np.add([1,2,3], [1,2,3], out=np.empty(3))
refcount: 2
array([ 2.,  4.,  6.])
>>> arr = np.empty(3)
>>> np.add([1,2,3], [1,2,3], out=arr)
refcount: 3
array([ 2.,  4.,  6.])

Based on this PR.

On Python 3.5 the same test gave different results:

Python 3.5.4 |Continuum Analytics, Inc.| (default, Aug 14 2017, 13:41:13) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> np.add([1,2,3], [1,2,3], out=np.empty(3))
refcount: 1
__main__:1: RuntimeWarning: the output array seems to be a new array, if the returned values of the ufunc are not assigned to a new variable the results may be lost
array([ 2.,  4.,  6.])
>>> arr = np.empty(3)
>>> np.add([1,2,3], [1,2,3], out=arr)
refcount: 2
array([ 2.,  4.,  6.])

MSeifert04 · 2017-09-06T23:28:40Z

@njsmith Yes, I was wondering. Not sure if it's maybe related to the _FastCall optimizations though.

MSeifert04 · 2017-09-08T16:18:34Z

Probably not a "good solution" but I skipped the assert on 3.6. At least it's passing the tests.

It would be possible to change it so that it asserts "no warning" on 3.6+ in case the reference counting is updated again and the out has the "expected reference count" again. Or should I just check for refcnt 2 in 3.6+?

njsmith · 2017-09-09T02:49:33Z

I suspect x = ...; np.add(..., out=x) will have refcnt 2 on cython.

I dunno, if this is fragile enough that our choices are to either encode quirky interpreter details, or else give up on supporting it on the most recent versions of Python, then is it worth doing? Presumably in a few years then all the Python versions we support will be like 3.6...

MSeifert04 · 2017-09-09T09:26:32Z

then is it worth doing?

Probably not.

njsmith · 2017-09-09T23:38:50Z

@MSeifert04 It was a nice idea; sorry it didn't work out.

MAINT: Warn if out argument has refcnt 1 and owns its data.

5951034

MSeifert04 commented Aug 31, 2017

View reviewed changes

seberg reviewed Sep 6, 2017

View reviewed changes

suppress_warnings instead of catch_warnings and exclude PyPy

226c5e9

Skip test for Warning on Python 3.6

9c05080

MSeifert04 closed this Sep 9, 2017

MSeifert04 deleted the testbranch branch September 11, 2017 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Warn if out argument has refcnt 1 and owns its data. #9632

MAINT: Warn if out argument has refcnt 1 and owns its data. #9632

MSeifert04 commented Aug 31, 2017 •

edited

MSeifert04 Aug 31, 2017

seberg Sep 6, 2017

seberg Sep 6, 2017

seberg Sep 6, 2017

MSeifert04 commented Aug 31, 2017 •

edited

seberg left a comment

seberg Sep 6, 2017

seberg Sep 6, 2017

seberg Sep 6, 2017

seberg Sep 6, 2017

MSeifert04 commented Sep 6, 2017 •

edited

MSeifert04 commented Sep 6, 2017

MSeifert04 commented Sep 6, 2017 •

edited

njsmith commented Sep 6, 2017

charris commented Sep 6, 2017 •

edited

MSeifert04 commented Sep 6, 2017 •

edited

MSeifert04 commented Sep 6, 2017

MSeifert04 commented Sep 8, 2017

njsmith commented Sep 9, 2017

MSeifert04 commented Sep 9, 2017

njsmith commented Sep 9, 2017

MAINT: Warn if out argument has refcnt 1 and owns its data. #9632

MAINT: Warn if out argument has refcnt 1 and owns its data. #9632

Conversation

MSeifert04 commented Aug 31, 2017 • edited

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

MSeifert04 commented Aug 31, 2017 • edited

seberg 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

MSeifert04 commented Sep 6, 2017 • edited

MSeifert04 commented Sep 6, 2017

MSeifert04 commented Sep 6, 2017 • edited

njsmith commented Sep 6, 2017

charris commented Sep 6, 2017 • edited

MSeifert04 commented Sep 6, 2017 • edited

MSeifert04 commented Sep 6, 2017

MSeifert04 commented Sep 8, 2017

njsmith commented Sep 9, 2017

MSeifert04 commented Sep 9, 2017

njsmith commented Sep 9, 2017

MSeifert04 commented Aug 31, 2017 •

edited

MSeifert04 commented Aug 31, 2017 •

edited

MSeifert04 commented Sep 6, 2017 •

edited

MSeifert04 commented Sep 6, 2017 •

edited

charris commented Sep 6, 2017 •

edited

MSeifert04 commented Sep 6, 2017 •

edited