BUG: test, fix PyArray_DiscardWritebackIfCopy refcount issue and docu… #10824
Conversation
91a7f71 to
c8bb7c5
Compare
| static NPY_INLINE void | ||
| PyArray_DiscardWritebackIfCopy(PyArrayObject *arr) | ||
| { | ||
| if (arr != NULL) { | ||
| PyArrayObject_fields *fa = (PyArrayObject_fields *)arr; | ||
| if ((PyArray_FLAGS(arr) & NPY_ARRAY_WRITEBACKIFCOPY) || | ||
| (PyArray_FLAGS(arr) & NPY_ARRAY_UPDATEIFCOPY)) { |
There was a problem hiding this comment.
Needs more indentation to separate it from the block. Alignment isn't desirable here.
| if ((PyArray_FLAGS(arr) & NPY_ARRAY_WRITEBACKIFCOPY) || | ||
| (PyArray_FLAGS(arr) & NPY_ARRAY_UPDATEIFCOPY)) { | ||
| PyArrayObject *base = (PyArrayObject *)PyArray_BASE(arr); |
There was a problem hiding this comment.
Why can't you continue using base?
There was a problem hiding this comment.
It looks to do the same thing, just in one step instead of two.
There was a problem hiding this comment.
fixed to be more like PyArray_ResolveWritebackIfCopy which this is meant to be like (except without the PyArray_CopyAnyInto)
numpy/core/tests/test_multiarray.py
Outdated
| @@ -7254,9 +7254,31 @@ def test_view_assign(self): | |||
| assert_equal(arr, -100) | |||
| # after resolve, the two arrays no longer reference each other | |||
| assert_(not arr_wb.ctypes.data == 0) | |||
| assert_equal(arr_wb.base, None) | |||
There was a problem hiding this comment.
Just curious, but shouldn't the base point to something valid even if the array is about to be destroyed?
There was a problem hiding this comment.
base is meant to be None for an array that owns its data, see documentation for base. The point of the test is to show that the two arrays arr_wb and arr are totally independent after the call to PyArray_ResolveWritebackIfCopy or PyArray_DiscardWritebackIfCopy.
numpy/core/tests/test_multiarray.py
Outdated
| arr_wb[:] = 100 | ||
| assert_equal(arr, -100) | ||
|
|
||
| def test_view_discard_refcount(self): | ||
| from numpy.core._multiarray_tests import npy_create_writebackifcopy, npy_discard | ||
| arr = np.arange(9).reshape(3, 3).T |
numpy/core/tests/test_multiarray.py
Outdated
| arr_wb = npy_create_writebackifcopy(arr) | ||
| assert_(arr_wb.flags.writebackifcopy) | ||
| assert_(arr_wb.base is arr) | ||
| arr_wb[:] = -100 |
There was a problem hiding this comment.
Maybe arr_wb[...] is clearer? The array has multiple dimensions.
numpy/core/tests/test_multiarray.py
Outdated
| # arr remains unchanged after discard | ||
| assert_equal(arr, orig) | ||
| # after discard, the two arrays no longer reference each other | ||
| assert_(not arr_wb.ctypes.data == 0) |
There was a problem hiding this comment.
not? I assume this is to check that the data pointer still exists, so why not !=?
numpy/core/tests/test_multiarray.py
Outdated
| assert_equal(arr_wb.base, None) | ||
| if HAS_REFCOUNT: | ||
| assert_equal(arr_cnt, sys.getrefcount(arr)) | ||
| arr_wb[:] = 100 |
There was a problem hiding this comment.
See above. I don't quite see what this tests except that you can use the array.
There was a problem hiding this comment.
added comments to try to clarify the intent.
I wanted to test that I can use arr_wb and that after the call to discard or resolve the changes to arr_wb are not transferred to arr`
|
What is the policy for inclusion in 1.15? This is non-critical but does fix a refcount leak |
|
Not everything that I think should go in is marked, but I went ahead and marked this :) |
6faeb31 to
d15c30f
Compare
|
Rebased off master to remove merge conflict |
| PyArray_ENABLEFLAGS(base, NPY_ARRAY_WRITEABLE); | ||
| PyArrayObject_fields *fa = (PyArrayObject_fields *)arr; | ||
| if (fa && fa->base) { | ||
| if ((fa->flags & NPY_ARRAY_UPDATEIFCOPY) || (fa->flags & NPY_ARRAY_WRITEBACKIFCOPY)) { |
There was a problem hiding this comment.
But now the line is too long :( Do it like
if ((fa->flags & NPY_ARRAY_UPDATEIFCOPY) ||
(fa->flags & NPY_ARRAY_WRITEBACKIFCOPY)) {
PyArray_ENABLEFLAGS((PyArrayObject*)fa->base, NPY_ARRAY_WRITEABLE);
This is in the C style guide. Given that python style is now going to breaking before the operator, we might also want to do it like
if ((fa->flags & NPY_ARRAY_UPDATEIFCOPY)
|| (fa->flags & NPY_ARRAY_WRITEBACKIFCOPY)) {
PyArray_ENABLEFLAGS((PyArrayObject*)fa->base, NPY_ARRAY_WRITEABLE);
|
Just a style nit. |
|
OK, I fixed it. Will put this in when the tests finish. |
|
@ahaldane This would be good to backport as it fixes a bug in a function new to 1.14. |
|
Will do |
PyArray_DiscardWritebackIfCopy was not properly detaching
selffromself->base, as a consequence it left a ref toself->basedangling. This tests and fixes the function, and more clearly documents bothPyArray_DiscardWritebackIfCopyandPyArray_ResolveWritebackIfCopy