From eea813b34d5ce366d4e08d2dd5cb10d36193e5ff Mon Sep 17 00:00:00 2001 From: Tyler Reddy Date: Mon, 27 May 2024 16:50:18 -0600 Subject: [PATCH 1/3] BUG: fancy indexing copy * Fixes gh-26542 * For these very simple advanced indexing cases, if the `result` and `self` arrays share the same data pointers, use a copy of `result` to assign values to `self`, otherwise the outcome of the fancy indexing suffers from mutation of `result` before the assignments are complete. --- numpy/_core/src/multiarray/mapping.c | 4 ++++ numpy/_core/tests/test_indexing.py | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/numpy/_core/src/multiarray/mapping.c b/numpy/_core/src/multiarray/mapping.c index 1861241a040e..08440176474b 100644 --- a/numpy/_core/src/multiarray/mapping.c +++ b/numpy/_core/src/multiarray/mapping.c @@ -1999,6 +1999,10 @@ array_assign_subscript(PyArrayObject *self, PyObject *ind, PyObject *op) goto fail; } + if (solve_may_share_memory(self, tmp_arr, 1) == 1) { + tmp_arr = (PyArrayObject *)PyArray_NewCopy(tmp_arr, NPY_ANYORDER); + } + /* trivial_set checks the index for us */ if (mapiter_trivial_set( self, ind, tmp_arr, is_aligned, &cast_info) < 0) { diff --git a/numpy/_core/tests/test_indexing.py b/numpy/_core/tests/test_indexing.py index bea1c1017fb2..8281e16b835d 100644 --- a/numpy/_core/tests/test_indexing.py +++ b/numpy/_core/tests/test_indexing.py @@ -133,6 +133,13 @@ def test_empty_fancy_index(self): b = np.array([]) assert_raises(IndexError, a.__getitem__, b) + def test_gh_26542(self): + a = np.array([0, 1, 2]) + idx = np.array([2, 1, 0]) + a[idx] = a + expected = np.array([2, 1, 0]) + assert_equal(a, expected) + def test_ellipsis_index(self): a = np.array([[1, 2, 3], [4, 5, 6], From bf5042d7d718625902c28fec506f9881677b43fa Mon Sep 17 00:00:00 2001 From: Tyler Reddy Date: Tue, 25 Jun 2024 14:56:56 -0600 Subject: [PATCH 2/3] MAINT, BUG: PR 26558 revisions * Avoid leaking the old `tmp_arr`, based on reviewer feedback. * Add a test for a similar 2D case that fails, then hoist `solve_may_share_memory()` check farther up in the control flow such that the test passes. * Add a reviewer-requested test for index overlap. --- numpy/_core/src/multiarray/mapping.c | 8 ++++---- numpy/_core/tests/test_indexing.py | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/numpy/_core/src/multiarray/mapping.c b/numpy/_core/src/multiarray/mapping.c index 08440176474b..fa40af24bc48 100644 --- a/numpy/_core/src/multiarray/mapping.c +++ b/numpy/_core/src/multiarray/mapping.c @@ -1960,6 +1960,10 @@ array_assign_subscript(PyArrayObject *self, PyObject *ind, PyObject *op) tmp_arr = (PyArrayObject *)op; } + if (tmp_arr && solve_may_share_memory(self, tmp_arr, 1) == 1) { + Py_SETREF(tmp_arr, (PyArrayObject *)PyArray_NewCopy(tmp_arr, NPY_ANYORDER)); + } + /* * Special case for very simple 1-d fancy indexing, which however * is quite common. This saves not only a lot of setup time in the @@ -1999,10 +2003,6 @@ array_assign_subscript(PyArrayObject *self, PyObject *ind, PyObject *op) goto fail; } - if (solve_may_share_memory(self, tmp_arr, 1) == 1) { - tmp_arr = (PyArrayObject *)PyArray_NewCopy(tmp_arr, NPY_ANYORDER); - } - /* trivial_set checks the index for us */ if (mapiter_trivial_set( self, ind, tmp_arr, is_aligned, &cast_info) < 0) { diff --git a/numpy/_core/tests/test_indexing.py b/numpy/_core/tests/test_indexing.py index 8281e16b835d..9611f75221d2 100644 --- a/numpy/_core/tests/test_indexing.py +++ b/numpy/_core/tests/test_indexing.py @@ -140,6 +140,21 @@ def test_gh_26542(self): expected = np.array([2, 1, 0]) assert_equal(a, expected) + def test_gh_26542_2d(self): + a = np.array([[0, 1, 2]]) + idx_row = np.zeros(3, dtype=int) + idx_col = np.array([2, 1, 0]) + a[idx_row, idx_col] = a + expected = np.array([[2, 1, 0]]) + assert_equal(a, expected) + + def test_gh_26542_index_overlap(self): + arr = np.arange(100) + expected_vals = np.copy(arr[:-10]) + arr[10:] = arr[:-10] + actual_vals = arr[10:] + assert_equal(actual_vals, expected_vals) + def test_ellipsis_index(self): a = np.array([[1, 2, 3], [4, 5, 6], From 8bd67794b97c6405a0f2ae0e5e2920a2768b47f8 Mon Sep 17 00:00:00 2001 From: Tyler Reddy Date: Wed, 3 Jul 2024 09:30:03 -0600 Subject: [PATCH 3/3] BUG: PR 26558 revisions * The usage of `solve_may_share_memory` in the above PR wasn't quite right since it ignored the case of failing to solve the overlap problem. This has been revised according to reviewer feedback. Co-authored-by: Sebastian Berg --- numpy/_core/src/multiarray/mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/numpy/_core/src/multiarray/mapping.c b/numpy/_core/src/multiarray/mapping.c index fa40af24bc48..e329a7a6758c 100644 --- a/numpy/_core/src/multiarray/mapping.c +++ b/numpy/_core/src/multiarray/mapping.c @@ -1960,7 +1960,7 @@ array_assign_subscript(PyArrayObject *self, PyObject *ind, PyObject *op) tmp_arr = (PyArrayObject *)op; } - if (tmp_arr && solve_may_share_memory(self, tmp_arr, 1) == 1) { + if (tmp_arr && solve_may_share_memory(self, tmp_arr, 1) != 0) { Py_SETREF(tmp_arr, (PyArrayObject *)PyArray_NewCopy(tmp_arr, NPY_ANYORDER)); }