From e2c0520f4f9da7fd4e9f472ced175cf95418a258 Mon Sep 17 00:00:00 2001 From: Luke Sewell Date: Wed, 2 Jun 2021 17:51:21 +0100 Subject: [PATCH 1/4] Fix segfaulting issue with reduce iterator --- bottleneck/include/iterators.h | 102 ++++++++++++++++++-------------- bottleneck/tests/reduce_test.py | 8 +++ 2 files changed, 65 insertions(+), 45 deletions(-) diff --git a/bottleneck/include/iterators.h b/bottleneck/include/iterators.h index 1f5120a184..60a34e64de 100644 --- a/bottleneck/include/iterators.h +++ b/bottleneck/include/iterators.h @@ -98,37 +98,59 @@ static inline void init_iter_all(iter *it, PyArrayObject *a, int ravel, int anyo it->ndim_m2 = -1; it->length = 1; it->astride = 0; - } else if (C_CONTIGUOUS(a) && !F_CONTIGUOUS(a)) { - /* The &&! in the next two else ifs is to deal with relaxed - * stride checking introduced in numpy 1.12.0; see gh #161 */ - it->ndim_m2 = -1; - it->axis = ndim - 1; - it->length = PyArray_SIZE(a); - it->astride = 0; - for (i = ndim - 1; i > -1; i--) { - /* protect against length zero strides such as in - * np.ones((2, 2))[..., np.newaxis] */ - if (strides[i] == 0) { - continue; + } else { + /* If strides aren't in descending order, some of the assumptions for C_CONTIGUOUS don't hold */ + int strides_descending = 1; + for (i = 1; i < ndim; i++) { + if (strides[i] > strides[i-1]) { + strides_descending = 0; + break; } - it->astride = strides[i]; - break; } - } else if (F_CONTIGUOUS(a) && !C_CONTIGUOUS(a)) { - if (anyorder || !ravel) { + + if (strides_descending && C_CONTIGUOUS(a) && !F_CONTIGUOUS(a)) { + + /* The &&! in the next two else ifs is to deal with relaxed + * stride checking introduced in numpy 1.12.0; see gh #161 */ it->ndim_m2 = -1; + it->axis = ndim - 1; it->length = PyArray_SIZE(a); it->astride = 0; - for (i = 0; i < ndim; i++) { + for (i = ndim - 1; i > -1; i--) { /* protect against length zero strides such as in - * np.ones((2, 2), order='F')[np.newaxis, ...] */ + * np.ones((2, 2))[..., np.newaxis] */ if (strides[i] == 0) { continue; } it->astride = strides[i]; break; } - } else { + } else if (F_CONTIGUOUS(a) && !C_CONTIGUOUS(a)) { + if (anyorder || !ravel) { + it->ndim_m2 = -1; + it->length = PyArray_SIZE(a); + it->astride = 0; + for (i = 0; i < ndim; i++) { + /* protect against length zero strides such as in + * np.ones((2, 2), order='F')[np.newaxis, ...] */ + if (strides[i] == 0) { + continue; + } + it->astride = strides[i]; + break; + } + } else { + it->ndim_m2 = -1; + if (anyorder) { + a = (PyArrayObject *)PyArray_Ravel(a, NPY_ANYORDER); + } else { + a = (PyArrayObject *)PyArray_Ravel(a, NPY_CORDER); + } + it->a_ravel = a; + it->length = PyArray_DIM(a, 0); + it->astride = PyArray_STRIDE(a, 0); + } + } else if (ravel) { it->ndim_m2 = -1; if (anyorder) { a = (PyArrayObject *)PyArray_Ravel(a, NPY_ANYORDER); @@ -138,34 +160,24 @@ static inline void init_iter_all(iter *it, PyArrayObject *a, int ravel, int anyo it->a_ravel = a; it->length = PyArray_DIM(a, 0); it->astride = PyArray_STRIDE(a, 0); - } - } else if (ravel) { - it->ndim_m2 = -1; - if (anyorder) { - a = (PyArrayObject *)PyArray_Ravel(a, NPY_ANYORDER); } else { - a = (PyArrayObject *)PyArray_Ravel(a, NPY_CORDER); - } - it->a_ravel = a; - it->length = PyArray_DIM(a, 0); - it->astride = PyArray_STRIDE(a, 0); - } else { - it->ndim_m2 = ndim - 2; - it->astride = strides[0]; - for (i = 1; i < ndim; i++) { - if (strides[i] < it->astride) { - it->astride = strides[i]; - it->axis = i; + it->ndim_m2 = ndim - 2; + it->astride = strides[0]; + for (i = 1; i < ndim; i++) { + if (strides[i] < it->astride) { + it->astride = strides[i]; + it->axis = i; + } } - } - it->length = shape[it->axis]; - for (i = 0; i < ndim; i++) { - if (i != it->axis) { - it->indices[j] = 0; - it->astrides[j] = strides[i]; - it->shape[j] = shape[i]; - it->nits *= shape[i]; - j++; + it->length = shape[it->axis]; + for (i = 0; i < ndim; i++) { + if (i != it->axis) { + it->indices[j] = 0; + it->astrides[j] = strides[i]; + it->shape[j] = shape[i]; + it->nits *= shape[i]; + j++; + } } } } diff --git a/bottleneck/tests/reduce_test.py b/bottleneck/tests/reduce_test.py index 56723e1d9a..75aec72e0d 100644 --- a/bottleneck/tests/reduce_test.py +++ b/bottleneck/tests/reduce_test.py @@ -236,3 +236,11 @@ def test_ddof_nans(func, dtype): for axis in [None, 0, 1, -1]: result = func(array, axis=axis, ddof=3) assert np.isnan(result) + + +@pytest.mark.parametrize("dtype", DTYPES) +@pytest.mark.parametrize("func", (bn.nanmean, bn.nanmax), ids=lambda x: x.__name__) +def test_reduce_with_unordered_strides(func, dtype) -> None: + array = np.zeros((1, 500, 2), dtype=dtype).transpose((1,2,0)) + result = func(array) + assert result == 0 From 65743d94a91496f700d5c176c75046c81d625c8b Mon Sep 17 00:00:00 2001 From: Luke Sewell Date: Wed, 28 Jul 2021 09:23:25 +0100 Subject: [PATCH 2/4] Fix more generically, using itemsize rather than checking strides --- bottleneck/include/iterators.h | 111 ++++++++++---------------------- bottleneck/tests/reduce_test.py | 14 +++- 2 files changed, 46 insertions(+), 79 deletions(-) diff --git a/bottleneck/include/iterators.h b/bottleneck/include/iterators.h index 60a34e64de..2f09f209da 100644 --- a/bottleneck/include/iterators.h +++ b/bottleneck/include/iterators.h @@ -98,86 +98,45 @@ static inline void init_iter_all(iter *it, PyArrayObject *a, int ravel, int anyo it->ndim_m2 = -1; it->length = 1; it->astride = 0; + } else if (F_CONTIGUOUS(a) && !C_CONTIGUOUS(a) && ravel && !anyorder) { + it->ndim_m2 = -1; + a = (PyArrayObject *)PyArray_Ravel(a, NPY_CORDER); + it->a_ravel = a; + it->length = PyArray_DIM(a, 0); + it->astride = PyArray_STRIDE(a, 0); + } else if (C_CONTIGUOUS(a) || F_CONTIGUOUS(a)) { + /* If continguous then we just need the itemsize */ + it->ndim_m2 = -1; + // it->axis does not matter + it->length = PyArray_SIZE(a); + it->astride = item_size; + } else if (ravel) { + it->ndim_m2 = -1; + if (anyorder) { + a = (PyArrayObject *)PyArray_Ravel(a, NPY_ANYORDER); + } else { + a = (PyArrayObject *)PyArray_Ravel(a, NPY_CORDER); + } + it->a_ravel = a; + it->length = PyArray_DIM(a, 0); + it->astride = PyArray_STRIDE(a, 0); } else { - /* If strides aren't in descending order, some of the assumptions for C_CONTIGUOUS don't hold */ - int strides_descending = 1; + it->ndim_m2 = ndim - 2; + it->astride = strides[0]; for (i = 1; i < ndim; i++) { - if (strides[i] > strides[i-1]) { - strides_descending = 0; - break; - } - } - - if (strides_descending && C_CONTIGUOUS(a) && !F_CONTIGUOUS(a)) { - - /* The &&! in the next two else ifs is to deal with relaxed - * stride checking introduced in numpy 1.12.0; see gh #161 */ - it->ndim_m2 = -1; - it->axis = ndim - 1; - it->length = PyArray_SIZE(a); - it->astride = 0; - for (i = ndim - 1; i > -1; i--) { - /* protect against length zero strides such as in - * np.ones((2, 2))[..., np.newaxis] */ - if (strides[i] == 0) { - continue; - } + if (strides[i] < it->astride) { it->astride = strides[i]; - break; - } - } else if (F_CONTIGUOUS(a) && !C_CONTIGUOUS(a)) { - if (anyorder || !ravel) { - it->ndim_m2 = -1; - it->length = PyArray_SIZE(a); - it->astride = 0; - for (i = 0; i < ndim; i++) { - /* protect against length zero strides such as in - * np.ones((2, 2), order='F')[np.newaxis, ...] */ - if (strides[i] == 0) { - continue; - } - it->astride = strides[i]; - break; - } - } else { - it->ndim_m2 = -1; - if (anyorder) { - a = (PyArrayObject *)PyArray_Ravel(a, NPY_ANYORDER); - } else { - a = (PyArrayObject *)PyArray_Ravel(a, NPY_CORDER); - } - it->a_ravel = a; - it->length = PyArray_DIM(a, 0); - it->astride = PyArray_STRIDE(a, 0); - } - } else if (ravel) { - it->ndim_m2 = -1; - if (anyorder) { - a = (PyArrayObject *)PyArray_Ravel(a, NPY_ANYORDER); - } else { - a = (PyArrayObject *)PyArray_Ravel(a, NPY_CORDER); + it->axis = i; } - it->a_ravel = a; - it->length = PyArray_DIM(a, 0); - it->astride = PyArray_STRIDE(a, 0); - } else { - it->ndim_m2 = ndim - 2; - it->astride = strides[0]; - for (i = 1; i < ndim; i++) { - if (strides[i] < it->astride) { - it->astride = strides[i]; - it->axis = i; - } - } - it->length = shape[it->axis]; - for (i = 0; i < ndim; i++) { - if (i != it->axis) { - it->indices[j] = 0; - it->astrides[j] = strides[i]; - it->shape[j] = shape[i]; - it->nits *= shape[i]; - j++; - } + } + it->length = shape[it->axis]; + for (i = 0; i < ndim; i++) { + if (i != it->axis) { + it->indices[j] = 0; + it->astrides[j] = strides[i]; + it->shape[j] = shape[i]; + it->nits *= shape[i]; + j++; } } } diff --git a/bottleneck/tests/reduce_test.py b/bottleneck/tests/reduce_test.py index 75aec72e0d..6d2ab27bc8 100644 --- a/bottleneck/tests/reduce_test.py +++ b/bottleneck/tests/reduce_test.py @@ -240,7 +240,15 @@ def test_ddof_nans(func, dtype): @pytest.mark.parametrize("dtype", DTYPES) @pytest.mark.parametrize("func", (bn.nanmean, bn.nanmax), ids=lambda x: x.__name__) -def test_reduce_with_unordered_strides(func, dtype) -> None: - array = np.zeros((1, 500, 2), dtype=dtype).transpose((1,2,0)) +def test_reduce_with_unordered_strides_ccontig(func, dtype) -> None: + array = np.ones((1, 500, 2), dtype=dtype).transpose((1,2,0)) result = func(array) - assert result == 0 + assert result == 1000 + +@pytest.mark.parametrize("dtype", DTYPES) +@pytest.mark.parametrize("func", (bn.nanmean, bn.nanmax), ids=lambda x: x.__name__) +def test_reduce_with_unordered_strides_fcontig(func, dtype) -> None: + array = np.ones((1, 500, 2), dtype=dtype).transpose((0,2,1)) + result = func(array) + assert result == 1000 + From 0cc52b5882e6d32ed643f45e54ca8470a93b1d86 Mon Sep 17 00:00:00 2001 From: Luke Sewell Date: Fri, 10 Sep 2021 13:27:41 +0100 Subject: [PATCH 3/4] Simplify iteration options for reduce_all --- bottleneck/include/iterators.h | 8 +------- bottleneck/tests/reduce_test.py | 24 ++++++++++++++++++------ 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/bottleneck/include/iterators.h b/bottleneck/include/iterators.h index 2f09f209da..20263bb96b 100644 --- a/bottleneck/include/iterators.h +++ b/bottleneck/include/iterators.h @@ -98,13 +98,7 @@ static inline void init_iter_all(iter *it, PyArrayObject *a, int ravel, int anyo it->ndim_m2 = -1; it->length = 1; it->astride = 0; - } else if (F_CONTIGUOUS(a) && !C_CONTIGUOUS(a) && ravel && !anyorder) { - it->ndim_m2 = -1; - a = (PyArrayObject *)PyArray_Ravel(a, NPY_CORDER); - it->a_ravel = a; - it->length = PyArray_DIM(a, 0); - it->astride = PyArray_STRIDE(a, 0); - } else if (C_CONTIGUOUS(a) || F_CONTIGUOUS(a)) { + } else if (C_CONTIGUOUS(a) || (anyorder && F_CONTIGUOUS(a))) { /* If continguous then we just need the itemsize */ it->ndim_m2 = -1; // it->axis does not matter diff --git a/bottleneck/tests/reduce_test.py b/bottleneck/tests/reduce_test.py index 6d2ab27bc8..02298dd1b0 100644 --- a/bottleneck/tests/reduce_test.py +++ b/bottleneck/tests/reduce_test.py @@ -239,16 +239,28 @@ def test_ddof_nans(func, dtype): @pytest.mark.parametrize("dtype", DTYPES) -@pytest.mark.parametrize("func", (bn.nanmean, bn.nanmax), ids=lambda x: x.__name__) -def test_reduce_with_unordered_strides_ccontig(func, dtype) -> None: +@pytest.mark.parametrize( + ("func", "expected"), + [(bn.nansum, 1000), + (bn.nanmean, 1), + (bn.nanmax, 1)], + ids=lambda x: x.__name__ if not isinstance(x, int) else x +) +def test_reduce_with_unordered_strides_ccontig(func, expected, dtype) -> None: array = np.ones((1, 500, 2), dtype=dtype).transpose((1,2,0)) result = func(array) - assert result == 1000 + assert result == expected @pytest.mark.parametrize("dtype", DTYPES) -@pytest.mark.parametrize("func", (bn.nanmean, bn.nanmax), ids=lambda x: x.__name__) -def test_reduce_with_unordered_strides_fcontig(func, dtype) -> None: +@pytest.mark.parametrize( + ("func", "expected"), + [(bn.nansum, 1000), + (bn.nanmean, 1), + (bn.nanmax, 1)], + ids=lambda x: x.__name__ if not isinstance(x, int) else x +) +def test_reduce_with_unordered_strides_fcontig(func, expected, dtype) -> None: array = np.ones((1, 500, 2), dtype=dtype).transpose((0,2,1)) result = func(array) - assert result == 1000 + assert result == expected From 3ab9eb809c5734643e833718fac4d28442ce3d3a Mon Sep 17 00:00:00 2001 From: Luke Sewell Date: Fri, 10 Sep 2021 13:45:38 +0100 Subject: [PATCH 4/4] Remove comment --- bottleneck/include/iterators.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/bottleneck/include/iterators.h b/bottleneck/include/iterators.h index 20263bb96b..8be840f779 100644 --- a/bottleneck/include/iterators.h +++ b/bottleneck/include/iterators.h @@ -87,9 +87,6 @@ static inline void init_iter_all(iter *it, PyArrayObject *a, int ravel, int anyo it->nits = 1; it->a_ravel = NULL; - /* The fix for relaxed strides checking in numpy and the fix for - * issue #183 has left this if..else tree in need of a refactor from the - * the ground up */ if (ndim == 1) { it->ndim_m2 = -1; it->length = shape[0];