BUG: Fixed deepcopy of object arrays. #6456

pelson · 2015-10-12T13:05:09Z

Fixes a bug with deepcopy of object arrays in F-order:

In [1]: import numpy as np

In [2]: np.__version__       
Out[2]: '1.11.0.dev0+9cc55dc'

In [3]: a = {'a': 1}

In [4]: b = {'b': 2}

In [5]: np.array([[a, b], [a, b]], order='F')
Out[5]: 
array([[{'a': 1}, {'b': 2}],
       [{'a': 1}, {'b': 2}]], dtype=object)

In [6]: import copy

In [7]: copy.deepcopy(np.array([[a, b], [a, b]], order='F'))
Out[7]: 
array([[{'a': 1}, {'a': 1}],
       [{'b': 2}, {'b': 2}]], dtype=object)

This was discovered as part of a bug found with biggus in SciTools/biggus#157.

pelson · 2015-10-12T13:06:49Z

numpy/core/tests/test_regression.py

@@ -457,7 +457,7 @@ def test_swap_real(self, level=rlevel):

    def test_object_array_from_list(self, level=rlevel):
        # Ticket #270
-        np.array([1, 'A', None])  # Should succeed


It does succeed - I just wanted it to make an assertion.
Happy to revert though.

pelson · 2015-10-12T13:15:37Z

numpy/core/src/multiarray/methods.c

@@ -1426,21 +1429,44 @@ array_deepcopy(PyArrayObject *self, PyObject *args)
        if (deepcopy == NULL) {
            return NULL;
        }
-        it = (PyArrayIterObject *)PyArray_IterNew((PyObject *)self);


The problem is that IterNew is iterating in C order, and optr is therefore getting out of synch.

jaimefrio · 2015-10-12T13:30:52Z

Not that I'm against using the new iterator, but wouldn't you get the same result if you changed this line:

_deepcopy_call(it->dataptr, optr, PyArray_DESCR(self), deepcopy, visit);

with

_deepcopy_call(it->dataptr, it->dataptr, PyArray_DESCR(self), deepcopy, visit);

and got rid of all the optr code, let the iterator handle all the iteration?

pelson · 2015-10-12T14:27:40Z

and got rid of all the optr code, let the iterator handle all the iteration?

I think you may be right. I was concerned about the fact that it is iterating over self, not the result array, but I think that is fine in this case. To be honest, I took this as an opportunity to understand the new iterator API - do you have a preference on my next steps - obviously, if I take the easier solution it will be easier to apply to a potential v1.10.1, but on the other hand if I stick with my original implementation it is making use of the new iterator API...

jaimefrio · 2015-10-12T16:54:37Z

numpy/core/src/multiarray/methods.c

-            _deepcopy_call(it->dataptr, optr, PyArray_DESCR(self), deepcopy, visit);
-            optr += PyArray_DESCR(self)->elsize;
-            PyArray_ITER_NEXT(it);
+        NpyIter_IterNextFunc *iternext = NpyIter_GetIterNext(iter, NULL);


This has to be declared at the beginning of the function to comply with the C89 strictness of MSVC.

All declarations must go at the beginning of a block.

jaimefrio · 2015-10-12T19:18:18Z

My personal view is that, for the new iterator to replace the old one, it needs a middle layer that makes implementing simple iterations like this one much simpler and less verbose: the alternative to the 26 lines that your PR adds to methods.c is to keep using the old iterator and remove 3. And there is no obvious benefit from the change, so I would not have included the new iterator as part of this PR.

Then again, this is your PR, not mine, and there is nothing fundamentally wrong with using NpyIter. It certainly should be encouraged for new development. So if you manage to fix the test errors, I wouldn't mind merging it as is.

charris · 2015-12-22T17:32:24Z

numpy/core/tests/test_regression.py

+        assert_equal(arr, arr_cp)
+        self.assertTrue(arr is not arr_cp)
+        # Ensure that we have actually copied the item.
+        self.assertIsNot(arr[0, 1], arr_cp[1, 1])


assertIs and assertIsNot are not available in Python 2.6, use assertTrue and assertFalse with proper arguments.

pelson · 2016-01-06T14:40:45Z

Thanks @jaimefrio and @charris - I've updated the PR based on your review. I've kept the new API implementation, but am still not averse to choosing the easier solution of updating the existing implementation (either way, we can keep the new tests).

jaimefrio · 2016-01-06T21:42:42Z

numpy/core/src/multiarray/methods.c


    if (!PyArg_ParseTuple(args, "O", &visit)) {
        return NULL;
    }
-    ret = (PyArrayObject *)PyArray_NewCopy(self, NPY_KEEPORDER);
+    copied_array = (PyArrayObject*) PyArray_NewCopy(self, NPY_KEEPORDER);


We should be checking that copied_array is not NULL, and if it is, return NULL to signal an error happened.

jaimefrio · 2016-01-06T21:51:06Z

There seemed to be a potential memory leak in case of failure. Was already there, but would be nice if you could fix it also as part of this PR.

pelson · 2016-01-07T08:59:50Z

There seemed to be a potential memory leak in case of failure. Was already there, but would be nice if you could fix it also as part of this PR.

Yep, that seems reasonable! I've added a commit (d158c59). Happy to squash if that is necessary.

pelson · 2016-01-07T09:01:25Z

Additionally, if there are stylistic improvements to the code, do let me know - I'm happy to fix them, and would appreciate critique.

pelson · 2016-01-18T10:52:49Z

Has this missed the v1.11 boat @charris?

charris · 2016-01-18T15:44:58Z

@pelson I'm going to leave this with @jaimefrio for when he gets back. Don't worry, we are shooting for branching 1.12 in April, not that far off.

homu · 2016-09-24T23:27:20Z

☔ The latest upstream changes (presumably #6054) made this pull request unmergeable. Please resolve the merge conflicts.

charris · 2016-09-24T23:31:44Z

Hmm, we should get this in. @pelson, could you rebase this?

charris · 2016-10-07T16:48:53Z

Squashed and rebased in #8125, so closing this. Thanks @pelson .

pelson · 2016-10-08T05:19:32Z

Thanks for taking this forward @charris. Great to have it fixed in master 👍

aojeda · 2017-01-31T01:38:11Z

Thanks guys for fixing the issue so quickly!
Onward!

charris · 2017-01-31T01:52:59Z

@aojeda Thanks, but this is the PR that caused your problem 8-)

aojeda · 2017-01-31T01:56:26Z

Ah ah true, I'll take it back for now then ;-)

pelson force-pushed the deepcopy_object branch from c34aa83 to 2112604 Compare October 12, 2015 13:05

pelson reviewed Oct 12, 2015
View reviewed changes

BUG: Fixed deepcopy of F-order object arrays.

2c24cbf

pelson force-pushed the deepcopy_object branch from 2112604 to 2c24cbf Compare October 12, 2015 13:07

pelson reviewed Oct 12, 2015
View reviewed changes

charris added 00 - Bug component: numpy._core labels Oct 12, 2015

jaimefrio reviewed Oct 12, 2015
View reviewed changes

charris reviewed Dec 22, 2015
View reviewed changes

MAINT: Review actions on deepcopy.

5dfe572

jaimefrio reviewed Jan 6, 2016
View reviewed changes

BUG: Fixed memory leak with deepcopy.

5ba71b1

pelson force-pushed the deepcopy_object branch from d158c59 to 5ba71b1 Compare January 18, 2016 09:27

charris modified the milestones: 1.11.2 release, 1.12.0 release Sep 24, 2016

charris mentioned this pull request Oct 7, 2016

Rebase, BUG: Fixed deepcopy of F-order object arrays. #8125

Merged

charris closed this Oct 7, 2016

pelson deleted the deepcopy_object branch October 8, 2016 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fixed deepcopy of object arrays. #6456

BUG: Fixed deepcopy of object arrays. #6456

pelson commented Oct 12, 2015

pelson Oct 12, 2015

pelson Oct 12, 2015

jaimefrio commented Oct 12, 2015

pelson commented Oct 12, 2015

jaimefrio Oct 12, 2015

charris Dec 22, 2015

jaimefrio commented Oct 12, 2015

charris Dec 22, 2015

pelson commented Jan 6, 2016

jaimefrio Jan 6, 2016

jaimefrio commented Jan 6, 2016

pelson commented Jan 7, 2016

pelson commented Jan 7, 2016

pelson commented Jan 18, 2016

charris commented Jan 18, 2016

homu commented Sep 24, 2016

charris commented Sep 24, 2016

charris commented Oct 7, 2016 •

edited

Loading

pelson commented Oct 8, 2016

aojeda commented Jan 31, 2017

charris commented Jan 31, 2017

aojeda commented Jan 31, 2017

BUG: Fixed deepcopy of object arrays. #6456

BUG: Fixed deepcopy of object arrays. #6456

Conversation

pelson commented Oct 12, 2015

pelson Oct 12, 2015

Choose a reason for hiding this comment

pelson Oct 12, 2015

Choose a reason for hiding this comment

jaimefrio commented Oct 12, 2015

pelson commented Oct 12, 2015

jaimefrio Oct 12, 2015

Choose a reason for hiding this comment

charris Dec 22, 2015

Choose a reason for hiding this comment

jaimefrio commented Oct 12, 2015

charris Dec 22, 2015

Choose a reason for hiding this comment

pelson commented Jan 6, 2016

jaimefrio Jan 6, 2016

Choose a reason for hiding this comment

jaimefrio commented Jan 6, 2016

pelson commented Jan 7, 2016

pelson commented Jan 7, 2016

pelson commented Jan 18, 2016

charris commented Jan 18, 2016

homu commented Sep 24, 2016

charris commented Sep 24, 2016

charris commented Oct 7, 2016 • edited Loading

pelson commented Oct 8, 2016

aojeda commented Jan 31, 2017

charris commented Jan 31, 2017

aojeda commented Jan 31, 2017

charris commented Oct 7, 2016 •

edited

Loading