From 905fe2c8506a4d5540a37b34c228f15dd3342484 Mon Sep 17 00:00:00 2001 From: Nikita Grigorian Date: Mon, 5 Feb 2024 14:37:52 -0800 Subject: [PATCH 1/6] Fixes bugs with `writable` flag setting `writable` flag was not being set correctly for indexing, real views, imaginary views, tranposes, and where shape is set directly Also fixes cases where flag could be overridden by functions with `out` kwarg --- dpctl/tensor/_clip.py | 6 ++++++ dpctl/tensor/_elementwise_common.py | 6 ++++++ dpctl/tensor/_linear_algebra_functions.py | 3 +++ dpctl/tensor/_usmarray.pyx | 25 +++++++++++++---------- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/dpctl/tensor/_clip.py b/dpctl/tensor/_clip.py index d95c0fa764..4833984a25 100644 --- a/dpctl/tensor/_clip.py +++ b/dpctl/tensor/_clip.py @@ -262,6 +262,9 @@ def _clip_none(x, val, out, order, _binary_fn): f"output array must be of usm_ndarray type, got {type(out)}" ) + if not out.flags.writable: + raise ValueError("provided `out` array is read-only") + if out.shape != res_shape: raise ValueError( "The shape of input and output arrays are inconsistent. " @@ -600,6 +603,9 @@ def clip(x, /, min=None, max=None, out=None, order="K"): f"{type(out)}" ) + if not out.flags.writable: + raise ValueError("provided `out` array is read-only") + if out.shape != res_shape: raise ValueError( "The shape of input and output arrays are " diff --git a/dpctl/tensor/_elementwise_common.py b/dpctl/tensor/_elementwise_common.py index 4f0e7bfd37..4a812309a1 100644 --- a/dpctl/tensor/_elementwise_common.py +++ b/dpctl/tensor/_elementwise_common.py @@ -202,6 +202,9 @@ def __call__(self, x, out=None, order="K"): f"output array must be of usm_ndarray type, got {type(out)}" ) + if not out.flags.writable: + raise ValueError("provided `out` array is read-only") + if out.shape != x.shape: raise ValueError( "The shape of input and output arrays are inconsistent. " @@ -601,6 +604,9 @@ def __call__(self, o1, o2, out=None, order="K"): f"output array must be of usm_ndarray type, got {type(out)}" ) + if not out.flags.writable: + raise ValueError("provided `out` array is read-only") + if out.shape != res_shape: raise ValueError( "The shape of input and output arrays are inconsistent. " diff --git a/dpctl/tensor/_linear_algebra_functions.py b/dpctl/tensor/_linear_algebra_functions.py index 0894ac2077..9f8aef8a48 100644 --- a/dpctl/tensor/_linear_algebra_functions.py +++ b/dpctl/tensor/_linear_algebra_functions.py @@ -738,6 +738,9 @@ def matmul(x1, x2, out=None, dtype=None, order="K"): f"output array must be of usm_ndarray type, got {type(out)}" ) + if not out.flags.writable: + raise ValueError("provided `out` array is read-only") + if out.shape != res_shape: raise ValueError( "The shape of input and output arrays are inconsistent. " diff --git a/dpctl/tensor/_usmarray.pyx b/dpctl/tensor/_usmarray.pyx index 67e144f798..e1a8eb85a8 100644 --- a/dpctl/tensor/_usmarray.pyx +++ b/dpctl/tensor/_usmarray.pyx @@ -546,7 +546,7 @@ cdef class usm_ndarray: PyMem_Free(self.shape_) if (self.strides_): PyMem_Free(self.strides_) - self.flags_ = contig_flag + self.flags_ = (contig_flag | (self.flags_ & USM_ARRAY_WRITABLE)) self.nd_ = new_nd self.shape_ = shape_ptr self.strides_ = strides_ptr @@ -725,13 +725,13 @@ cdef class usm_ndarray: buffer=self.base_, offset=_meta[2] ) - res.flags_ |= (self.flags_ & USM_ARRAY_WRITABLE) res.array_namespace_ = self.array_namespace_ adv_ind = _meta[3] adv_ind_start_p = _meta[4] if adv_ind_start_p < 0: + res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) return res from ._copy_utils import _extract_impl, _nonzero_impl, _take_multi_index @@ -749,6 +749,7 @@ cdef class usm_ndarray: if not matching: raise IndexError("boolean index did not match indexed array in dimensions") res = _extract_impl(res, key_, axis=adv_ind_start_p) + res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) return res if any(ind.dtype == dpt_bool for ind in adv_ind): @@ -758,10 +759,13 @@ cdef class usm_ndarray: adv_ind_int.extend(_nonzero_impl(ind)) else: adv_ind_int.append(ind) - return _take_multi_index(res, tuple(adv_ind_int), adv_ind_start_p) - - return _take_multi_index(res, adv_ind, adv_ind_start_p) + res = _take_multi_index(res, tuple(adv_ind_int), adv_ind_start_p) + res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) + return res + res = _take_multi_index(res, adv_ind, adv_ind_start_p) + res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) + return res def to_device(self, target, stream=None): """ to_device(target_device) @@ -1040,8 +1044,7 @@ cdef class usm_ndarray: buffer=self.base_, offset=_meta[2], ) - # set flags and namespace - Xv.flags_ |= (self.flags_ & USM_ARRAY_WRITABLE) + # set namespace Xv.array_namespace_ = self.array_namespace_ from ._copy_utils import ( @@ -1225,7 +1228,7 @@ cdef usm_ndarray _real_view(usm_ndarray ary): offset=offset_elems, order=('C' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'F') ) - r.flags_ |= (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) r.array_namespace_ = ary.array_namespace_ return r @@ -1257,7 +1260,7 @@ cdef usm_ndarray _imag_view(usm_ndarray ary): offset=offset_elems, order=('C' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'F') ) - r.flags_ |= (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) r.array_namespace_ = ary.array_namespace_ return r @@ -1277,7 +1280,7 @@ cdef usm_ndarray _transpose(usm_ndarray ary): order=('F' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'C'), offset=ary.get_offset() ) - r.flags_ |= (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) return r @@ -1294,7 +1297,7 @@ cdef usm_ndarray _m_transpose(usm_ndarray ary): order=('F' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'C'), offset=ary.get_offset() ) - r.flags_ |= (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) return r From 87cd798b1f528bce63009650e7e3e499baa2ba21 Mon Sep 17 00:00:00 2001 From: Nikita Grigorian Date: Mon, 5 Feb 2024 14:37:57 -0800 Subject: [PATCH 2/6] Adds a test for writable flag view behavior --- dpctl/tests/test_usm_ndarray_ctor.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/dpctl/tests/test_usm_ndarray_ctor.py b/dpctl/tests/test_usm_ndarray_ctor.py index 7c5765332b..51f78eb59c 100644 --- a/dpctl/tests/test_usm_ndarray_ctor.py +++ b/dpctl/tests/test_usm_ndarray_ctor.py @@ -129,6 +129,25 @@ def test_usm_ndarray_flags_bug_gh_1334(): assert r.flags["F"] and r.flags["C"] +def test_usm_ndarray_writable_flag_views(): + get_queue_or_skip() + a = dpt.arange(10, dtype="f4") + a.flags["W"] = False + + a.shape = (5, 2) + assert not a.flags.writable + assert not a.T.flags.writable + assert not a.mT.flags.writable + assert not a.real.flags.writable + assert not a[0:3].flags.writable + + a = dpt.arange(10, dtype="c8") + a.flags["W"] = False + + assert not a.real.flags.writable + assert not a.imag.flags.writable + + @pytest.mark.parametrize( "dtype", [ From 71408b0ac0e6c9953421d93ec968d37cf5af27af Mon Sep 17 00:00:00 2001 From: Nikita Grigorian Date: Mon, 5 Feb 2024 15:04:30 -0800 Subject: [PATCH 3/6] Removes assumption that new array is writable Now flags are set based on input regardless of whether a new array is writable per review feedback --- dpctl/tensor/_usmarray.pyx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dpctl/tensor/_usmarray.pyx b/dpctl/tensor/_usmarray.pyx index e1a8eb85a8..a3d77de44c 100644 --- a/dpctl/tensor/_usmarray.pyx +++ b/dpctl/tensor/_usmarray.pyx @@ -731,7 +731,7 @@ cdef class usm_ndarray: adv_ind_start_p = _meta[4] if adv_ind_start_p < 0: - res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) return res from ._copy_utils import _extract_impl, _nonzero_impl, _take_multi_index @@ -749,7 +749,7 @@ cdef class usm_ndarray: if not matching: raise IndexError("boolean index did not match indexed array in dimensions") res = _extract_impl(res, key_, axis=adv_ind_start_p) - res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) return res if any(ind.dtype == dpt_bool for ind in adv_ind): @@ -760,11 +760,11 @@ cdef class usm_ndarray: else: adv_ind_int.append(ind) res = _take_multi_index(res, tuple(adv_ind_int), adv_ind_start_p) - res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) return res res = _take_multi_index(res, adv_ind, adv_ind_start_p) - res.flags_ ^= (~self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) return res def to_device(self, target, stream=None): @@ -1228,7 +1228,7 @@ cdef usm_ndarray _real_view(usm_ndarray ary): offset=offset_elems, order=('C' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'F') ) - r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) r.array_namespace_ = ary.array_namespace_ return r @@ -1260,7 +1260,7 @@ cdef usm_ndarray _imag_view(usm_ndarray ary): offset=offset_elems, order=('C' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'F') ) - r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) r.array_namespace_ = ary.array_namespace_ return r @@ -1280,7 +1280,7 @@ cdef usm_ndarray _transpose(usm_ndarray ary): order=('F' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'C'), offset=ary.get_offset() ) - r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) return r @@ -1297,7 +1297,7 @@ cdef usm_ndarray _m_transpose(usm_ndarray ary): order=('F' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'C'), offset=ary.get_offset() ) - r.flags_ ^= (~ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) return r From da413a68ff40d06c2f60c405087a444805a42a48 Mon Sep 17 00:00:00 2001 From: Nikita Grigorian Date: Mon, 5 Feb 2024 15:25:56 -0800 Subject: [PATCH 4/6] Adds _copy_writable for copying the writable flag between arrays --- dpctl/tensor/_usmarray.pyx | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/dpctl/tensor/_usmarray.pyx b/dpctl/tensor/_usmarray.pyx index a3d77de44c..1eef163dad 100644 --- a/dpctl/tensor/_usmarray.pyx +++ b/dpctl/tensor/_usmarray.pyx @@ -60,6 +60,9 @@ cdef object _as_zero_dim_ndarray(object usm_ary): view.shape = tuple() return view +cdef int _copy_writable(int lhs_flags, int rhs_flags): + "Copy the WRITABLE flag to lhs_flags from rhs_flags" + return (lhs_flag & ~USM_ARRAY_WRITABLE) | (rhs_flag & USM_ARRAY_WRITABLE) cdef class usm_ndarray: """ usm_ndarray(shape, dtype=None, strides=None, buffer="device", \ @@ -731,7 +734,7 @@ cdef class usm_ndarray: adv_ind_start_p = _meta[4] if adv_ind_start_p < 0: - res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = _copy_writable(res.flags_, self.flags_) return res from ._copy_utils import _extract_impl, _nonzero_impl, _take_multi_index @@ -749,7 +752,7 @@ cdef class usm_ndarray: if not matching: raise IndexError("boolean index did not match indexed array in dimensions") res = _extract_impl(res, key_, axis=adv_ind_start_p) - res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = _copy_writable(res.flags_, self.flags_) return res if any(ind.dtype == dpt_bool for ind in adv_ind): @@ -760,11 +763,11 @@ cdef class usm_ndarray: else: adv_ind_int.append(ind) res = _take_multi_index(res, tuple(adv_ind_int), adv_ind_start_p) - res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = _copy_writable(res.flags_, self.flags_) return res res = _take_multi_index(res, adv_ind, adv_ind_start_p) - res.flags_ = (res.flags_ & ~USM_ARRAY_WRITABLE) | (self.flags_ & USM_ARRAY_WRITABLE) + res.flags_ = _copy_writable(res.flags_, self.flags_) return res def to_device(self, target, stream=None): @@ -1228,7 +1231,7 @@ cdef usm_ndarray _real_view(usm_ndarray ary): offset=offset_elems, order=('C' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'F') ) - r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = _copy_writable(r.flags_, ary.flags_) r.array_namespace_ = ary.array_namespace_ return r @@ -1260,7 +1263,7 @@ cdef usm_ndarray _imag_view(usm_ndarray ary): offset=offset_elems, order=('C' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'F') ) - r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = _copy_writable(r.flags_, ary.flags_) r.array_namespace_ = ary.array_namespace_ return r @@ -1280,7 +1283,7 @@ cdef usm_ndarray _transpose(usm_ndarray ary): order=('F' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'C'), offset=ary.get_offset() ) - r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = _copy_writable(r.flags_, ary.flags_) return r @@ -1297,7 +1300,7 @@ cdef usm_ndarray _m_transpose(usm_ndarray ary): order=('F' if (ary.flags_ & USM_ARRAY_C_CONTIGUOUS) else 'C'), offset=ary.get_offset() ) - r.flags_ = (r.flags_ & ~USM_ARRAY_WRITABLE) | (ary.flags_ & USM_ARRAY_WRITABLE) + r.flags_ = _copy_writable(r.flags_, ary.flags_) return r From 8ac67c7dba85438007697af53f81fb449c7bc204 Mon Sep 17 00:00:00 2001 From: Nikita Grigorian Date: Mon, 5 Feb 2024 15:36:56 -0800 Subject: [PATCH 5/6] Correct typos in _copy_writable --- dpctl/tensor/_usmarray.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dpctl/tensor/_usmarray.pyx b/dpctl/tensor/_usmarray.pyx index 1eef163dad..5a8220db0c 100644 --- a/dpctl/tensor/_usmarray.pyx +++ b/dpctl/tensor/_usmarray.pyx @@ -62,7 +62,7 @@ cdef object _as_zero_dim_ndarray(object usm_ary): cdef int _copy_writable(int lhs_flags, int rhs_flags): "Copy the WRITABLE flag to lhs_flags from rhs_flags" - return (lhs_flag & ~USM_ARRAY_WRITABLE) | (rhs_flag & USM_ARRAY_WRITABLE) + return (lhs_flags & ~USM_ARRAY_WRITABLE) | (rhs_flags & USM_ARRAY_WRITABLE) cdef class usm_ndarray: """ usm_ndarray(shape, dtype=None, strides=None, buffer="device", \ From 356cf22687349cec2501fafa6e8eeaa22256e72e Mon Sep 17 00:00:00 2001 From: Nikita Grigorian Date: Mon, 5 Feb 2024 17:19:20 -0800 Subject: [PATCH 6/6] Fixes clip writing to read-only out arrays when min and max are none --- dpctl/tensor/_clip.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dpctl/tensor/_clip.py b/dpctl/tensor/_clip.py index 4833984a25..9a310df618 100644 --- a/dpctl/tensor/_clip.py +++ b/dpctl/tensor/_clip.py @@ -440,6 +440,9 @@ def clip(x, /, min=None, max=None, out=None, order="K"): f"{type(out)}" ) + if not out.flags.writable: + raise ValueError("provided `out` array is read-only") + if out.shape != x.shape: raise ValueError( "The shape of input and output arrays are "