From d58cba5108e9f59dacc32d6e3382fd6fe27cd3ee Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Fri, 18 Oct 2024 06:49:57 -0500 Subject: [PATCH 1/7] Roll must reduce shift steps by size along axis Closes gh-1857 Test added based on example provided in the issue. --- dpctl/tensor/_manipulation_functions.py | 8 +++++--- dpctl/tests/test_usm_ndarray_manipulation.py | 10 ++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/dpctl/tensor/_manipulation_functions.py b/dpctl/tensor/_manipulation_functions.py index bed326f4b0..283c8e8701 100644 --- a/dpctl/tensor/_manipulation_functions.py +++ b/dpctl/tensor/_manipulation_functions.py @@ -356,7 +356,7 @@ def roll(X, /, shift, *, axis=None): hev, roll_ev = ti._copy_usm_ndarray_for_roll_1d( src=X, dst=res, - shift=shift, + shift=(shift % X.size), sycl_queue=exec_q, depends=dep_evs, ) @@ -369,9 +369,11 @@ def roll(X, /, shift, *, axis=None): shifts = [ 0, ] * X.ndim + shape = X.shape for sh, ax in broadcasted: - shifts[ax] += sh - + n_i = shape[ax] + if n_i > 0: + shifts[ax] = int(shifts[ax] + sh) % int(n_i) res = dpt.empty( X.shape, dtype=X.dtype, usm_type=X.usm_type, sycl_queue=exec_q ) diff --git a/dpctl/tests/test_usm_ndarray_manipulation.py b/dpctl/tests/test_usm_ndarray_manipulation.py index 17262e2141..03e8e829cf 100644 --- a/dpctl/tests/test_usm_ndarray_manipulation.py +++ b/dpctl/tests/test_usm_ndarray_manipulation.py @@ -657,6 +657,16 @@ def test_roll_2d(data): assert_array_equal(Ynp, dpt.asnumpy(Y)) +def test_roll_out_bounds_shifts(): + "See gh-1857" + get_queue_or_skip() + + x = dpt.arange(4) + y = dpt.roll(x, np.uint64(2**63 + 2)) + expected = dpt.roll(x, 2) + assert dpt.all(y == expected) + + def test_roll_validation(): get_queue_or_skip() From a3d35095c8505491119844846cba58cfaa84cdcf Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Fri, 18 Oct 2024 08:15:53 -0500 Subject: [PATCH 2/7] Change to avoid branch to fix decrease in coverage --- dpctl/tensor/_manipulation_functions.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dpctl/tensor/_manipulation_functions.py b/dpctl/tensor/_manipulation_functions.py index 283c8e8701..2965653400 100644 --- a/dpctl/tensor/_manipulation_functions.py +++ b/dpctl/tensor/_manipulation_functions.py @@ -372,8 +372,7 @@ def roll(X, /, shift, *, axis=None): shape = X.shape for sh, ax in broadcasted: n_i = shape[ax] - if n_i > 0: - shifts[ax] = int(shifts[ax] + sh) % int(n_i) + shifts[ax] = int(shifts[ax] + sh) % int(n_i) if n_i > 0 else 0 res = dpt.empty( X.shape, dtype=X.dtype, usm_type=X.usm_type, sycl_queue=exec_q ) From 6054b1f74875f44d844a7b6c41eacb457f507f88 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Fri, 18 Oct 2024 15:19:46 -0500 Subject: [PATCH 3/7] Fix division by zero exception found by array API tests suite --- dpctl/tensor/_manipulation_functions.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dpctl/tensor/_manipulation_functions.py b/dpctl/tensor/_manipulation_functions.py index 2965653400..b68d3c6ff3 100644 --- a/dpctl/tensor/_manipulation_functions.py +++ b/dpctl/tensor/_manipulation_functions.py @@ -353,10 +353,12 @@ def roll(X, /, shift, *, axis=None): res = dpt.empty( X.shape, dtype=X.dtype, usm_type=X.usm_type, sycl_queue=exec_q ) + sz = X.size + shift = (shift % sz) if sz > 0 else 0 hev, roll_ev = ti._copy_usm_ndarray_for_roll_1d( src=X, dst=res, - shift=(shift % X.size), + shift=shift, sycl_queue=exec_q, depends=dep_evs, ) From 275fdba0db4f3a13c234f50b2ac2a15b5115b590 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Fri, 18 Oct 2024 15:20:09 -0500 Subject: [PATCH 4/7] Added tests based on division by zero test failure --- dpctl/tests/test_usm_ndarray_manipulation.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/dpctl/tests/test_usm_ndarray_manipulation.py b/dpctl/tests/test_usm_ndarray_manipulation.py index 03e8e829cf..882a001827 100644 --- a/dpctl/tests/test_usm_ndarray_manipulation.py +++ b/dpctl/tests/test_usm_ndarray_manipulation.py @@ -666,6 +666,20 @@ def test_roll_out_bounds_shifts(): expected = dpt.roll(x, 2) assert dpt.all(y == expected) + x_empty = x[1:1] + y = dpt.roll(x_empty, 11) + assert y.size == 0 + + x_2d = dpt.reshape(x, (2, 2)) + y = dpt.roll(x_2d, np.uint64(2**63 + 1), axis=1) + expected = dpt.roll(x_2d, 1, axis=1) + assert dpt.all(y == expected) + + x_2d_empty = x_2d[:, 1:1] + y = dpt.roll(x_2d_empty, 3, axis=1) + expected = dpt.empty_like(x_2d_empty) + assert dpt.all(y == expected) + def test_roll_validation(): get_queue_or_skip() From 10f5706f569739117aad00e3a9bd117663be35b6 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Sat, 19 Oct 2024 10:08:35 -0500 Subject: [PATCH 5/7] Renamed X to x as per docstring, add parens to inline if --- dpctl/tensor/_manipulation_functions.py | 28 ++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/dpctl/tensor/_manipulation_functions.py b/dpctl/tensor/_manipulation_functions.py index b68d3c6ff3..7a8888a980 100644 --- a/dpctl/tensor/_manipulation_functions.py +++ b/dpctl/tensor/_manipulation_functions.py @@ -311,7 +311,7 @@ def flip(X, /, *, axis=None): return X[indexer] -def roll(X, /, shift, *, axis=None): +def roll(x, /, shift, *, axis=None): """ roll(x, shift, axis) @@ -343,20 +343,20 @@ def roll(X, /, shift, *, axis=None): `device` attributes as `x` and whose elements are shifted relative to `x`. """ - if not isinstance(X, dpt.usm_ndarray): - raise TypeError(f"Expected usm_ndarray type, got {type(X)}.") - exec_q = X.sycl_queue + if not isinstance(x, dpt.usm_ndarray): + raise TypeError(f"Expected usm_ndarray type, got {type(x)}.") + exec_q = x.sycl_queue _manager = dputils.SequentialOrderManager[exec_q] if axis is None: shift = operator.index(shift) - dep_evs = _manager.submitted_events res = dpt.empty( - X.shape, dtype=X.dtype, usm_type=X.usm_type, sycl_queue=exec_q + x.shape, dtype=x.dtype, usm_type=x.usm_type, sycl_queue=exec_q ) - sz = X.size + sz = x.size shift = (shift % sz) if sz > 0 else 0 + dep_evs = _manager.submitted_events hev, roll_ev = ti._copy_usm_ndarray_for_roll_1d( - src=X, + src=x, dst=res, shift=shift, sycl_queue=exec_q, @@ -364,23 +364,23 @@ def roll(X, /, shift, *, axis=None): ) _manager.add_event_pair(hev, roll_ev) return res - axis = normalize_axis_tuple(axis, X.ndim, allow_duplicate=True) + axis = normalize_axis_tuple(axis, x.ndim, allow_duplicate=True) broadcasted = np.broadcast(shift, axis) if broadcasted.ndim > 1: raise ValueError("'shift' and 'axis' should be scalars or 1D sequences") shifts = [ 0, - ] * X.ndim - shape = X.shape + ] * x.ndim + shape = x.shape for sh, ax in broadcasted: n_i = shape[ax] - shifts[ax] = int(shifts[ax] + sh) % int(n_i) if n_i > 0 else 0 + shifts[ax] = (int(shifts[ax] + sh) % int(n_i)) if n_i > 0 else 0 res = dpt.empty( - X.shape, dtype=X.dtype, usm_type=X.usm_type, sycl_queue=exec_q + x.shape, dtype=x.dtype, usm_type=x.usm_type, sycl_queue=exec_q ) dep_evs = _manager.submitted_events ht_e, roll_ev = ti._copy_usm_ndarray_for_roll_nd( - src=X, dst=res, shifts=shifts, sycl_queue=exec_q, depends=dep_evs + src=x, dst=res, shifts=shifts, sycl_queue=exec_q, depends=dep_evs ) _manager.add_event_pair(ht_e, roll_ev) return res From 23fd9868eef9d7ec1f42adf3d523d7453a4d5139 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Sat, 19 Oct 2024 12:45:24 -0500 Subject: [PATCH 6/7] Corrected select statement for shift normalization --- dpctl/tensor/libtensor/source/copy_for_roll.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dpctl/tensor/libtensor/source/copy_for_roll.cpp b/dpctl/tensor/libtensor/source/copy_for_roll.cpp index 774228c6a7..b624d02882 100644 --- a/dpctl/tensor/libtensor/source/copy_for_roll.cpp +++ b/dpctl/tensor/libtensor/source/copy_for_roll.cpp @@ -326,7 +326,7 @@ copy_usm_ndarray_for_roll_nd(const dpctl::tensor::usm_ndarray &src, // normalize shift parameter to be 0 <= offset < dim py::ssize_t dim = src_shape_ptr[i]; size_t offset = - (shifts[i] > 0) ? (shifts[i] % dim) : dim + (shifts[i] % dim); + (shifts[i] >= 0) ? (shifts[i] % dim) : dim + (shifts[i] % dim); normalized_shifts.push_back(offset); } From bd0c9b28bf743f8bc3d508b196826c7b0d5339b2 Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk Date: Sun, 20 Oct 2024 16:54:19 -0500 Subject: [PATCH 7/7] Use operator.index to normalize shift --- dpctl/tensor/_manipulation_functions.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/dpctl/tensor/_manipulation_functions.py b/dpctl/tensor/_manipulation_functions.py index 7a8888a980..f98a09ca72 100644 --- a/dpctl/tensor/_manipulation_functions.py +++ b/dpctl/tensor/_manipulation_functions.py @@ -352,7 +352,7 @@ def roll(x, /, shift, *, axis=None): res = dpt.empty( x.shape, dtype=x.dtype, usm_type=x.usm_type, sycl_queue=exec_q ) - sz = x.size + sz = operator.index(x.size) shift = (shift % sz) if sz > 0 else 0 dep_evs = _manager.submitted_events hev, roll_ev = ti._copy_usm_ndarray_for_roll_1d( @@ -373,8 +373,9 @@ def roll(x, /, shift, *, axis=None): ] * x.ndim shape = x.shape for sh, ax in broadcasted: - n_i = shape[ax] - shifts[ax] = (int(shifts[ax] + sh) % int(n_i)) if n_i > 0 else 0 + n_i = operator.index(shape[ax]) + shifted = shifts[ax] + operator.index(sh) + shifts[ax] = (shifted % n_i) if n_i > 0 else 0 res = dpt.empty( x.shape, dtype=x.dtype, usm_type=x.usm_type, sycl_queue=exec_q )