From 9922bd4125a202a22cd926b57b4f531ea7d5e7d2 Mon Sep 17 00:00:00 2001 From: liym27 <33742067+liym27@users.noreply.github.com> Date: Wed, 6 Jan 2021 11:45:05 +0800 Subject: [PATCH] Fix bug: In dynamic mode, if start or end is negetive, __getitem__ return wrong result(#30003) 1. when slice_item is a slice: 1) the start of __getitem__ should be std::max(start, 0) if slice 2) the start of __getitem__ should be std::min(end, dim) 2. when slice_item is an integer, it should be in [-dim_len, dim_len) 3. Fix error message to use accurate data --- paddle/fluid/operators/slice_op.cc | 7 +++++-- paddle/fluid/operators/slice_op.h | 8 ++++---- paddle/fluid/pybind/imperative.cc | 5 ++++- python/paddle/fluid/tests/unittests/test_var_base.py | 7 ++++++- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/paddle/fluid/operators/slice_op.cc b/paddle/fluid/operators/slice_op.cc index 8560f1f714d..b49e026b5e2 100644 --- a/paddle/fluid/operators/slice_op.cc +++ b/paddle/fluid/operators/slice_op.cc @@ -121,8 +121,11 @@ class SliceOp : public framework::OperatorWithKernel { start = std::max(start, 0); end = std::max(end, 0); end = std::min(end, dim_value); - PADDLE_ENFORCE_GT(end, start, platform::errors::InvalidArgument( - "end should greater than start")); + PADDLE_ENFORCE_GT(end, start, + platform::errors::InvalidArgument( + "end should greater than start, but received " + "end = %d, start = %d.", + ends[i], starts[i])); out_dims[axes[i]] = end - start; } } diff --git a/paddle/fluid/operators/slice_op.h b/paddle/fluid/operators/slice_op.h index 4de5c1f7508..9c30c4e07fa 100644 --- a/paddle/fluid/operators/slice_op.h +++ b/paddle/fluid/operators/slice_op.h @@ -122,8 +122,8 @@ class SliceKernel : public framework::OpKernel { PADDLE_ENFORCE_GT(end, start, platform::errors::InvalidArgument( "Attr(ends) should be greater than attr(starts) in " - "slice op. But received ends = %d, starts = %d.", - end, start)); + "slice op. But received end = %d, start = %d.", + ends[0], starts[0])); int64_t out_size = end - start; if (out_is_tensor_array) { @@ -181,8 +181,8 @@ class SliceKernel : public framework::OpKernel { end, start, platform::errors::InvalidArgument( "Attr(ends) should be greater than attr(starts) in " - "slice op. But received ends = %d, starts = %d.", - end, start)); + "slice op. But received end = %d, start = %d.", + ends[i], starts[i])); out_dims[axes[i]] = end - start; } } diff --git a/paddle/fluid/pybind/imperative.cc b/paddle/fluid/pybind/imperative.cc index 56c6020afeb..25ade963cbe 100644 --- a/paddle/fluid/pybind/imperative.cc +++ b/paddle/fluid/pybind/imperative.cc @@ -20,6 +20,7 @@ limitations under the License. */ #include #include +#include #include #include #include @@ -322,6 +323,7 @@ static int _PySlice_GetIndices(PySliceObject *r, Py_ssize_t length, std::string(Py_TYPE(r->start)->tp_name))); } if (*start < 0) *start += length; + *start = std::max(*start, static_cast(0)); } if (r->stop == Py_None) { *stop = *step < 0 ? -1 : length; @@ -335,6 +337,7 @@ static int _PySlice_GetIndices(PySliceObject *r, Py_ssize_t length, std::string(Py_TYPE(r->stop)->tp_name))); } if (*stop < 0) *stop += length; + *stop = std::min(*stop, length); } if (*stop > length) return -1; if (*start >= length) return -1; @@ -380,7 +383,7 @@ static void ParseIndexingSlice(framework::LoDTensor *tensor, PyObject *_index, int start = static_cast(PyLong_AsLong(slice_item)); auto s_t = start; start = start < 0 ? start + dim_len : start; - if (start >= dim_len) { + if (start >= dim_len || start < 0) { std::string str_error_message = "The starting index " + std::to_string(s_t) + " of slice is out of bounds in tensor " + std::to_string(dim) + diff --git a/python/paddle/fluid/tests/unittests/test_var_base.py b/python/paddle/fluid/tests/unittests/test_var_base.py index 06009e4ba8b..653127319a1 100644 --- a/python/paddle/fluid/tests/unittests/test_var_base.py +++ b/python/paddle/fluid/tests/unittests/test_var_base.py @@ -413,10 +413,11 @@ class TestVarBase(unittest.TestCase): var13 = var[2:10, 2:, -2:-1] var14 = var[1:-1, 0:2, ::-1] var15 = var[::-1, ::-1, ::-1] + var16 = var[-4:4] vars = [ var, var1, var2, var3, var4, var5, var6, var7, var8, var9, var10, - var11, var12, var13, var14, var15 + var11, var12, var13, var14, var15, var16 ] local_out = [var.numpy() for var in vars] @@ -444,6 +445,7 @@ class TestVarBase(unittest.TestCase): np.array_equal(local_out[14], tensor_array[1:-1, 0:2, ::-1])) self.assertTrue( np.array_equal(local_out[15], tensor_array[::-1, ::-1, ::-1])) + self.assertTrue(np.array_equal(local_out[16], tensor_array[-4:4])) def _test_for_var(self): np_value = np.random.random((30, 100, 100)).astype('float32') @@ -464,6 +466,9 @@ class TestVarBase(unittest.TestCase): with self.assertRaises(IndexError): y = var[self.shape[0]] + with self.assertRaises(IndexError): + y = var[0 - self.shape[0] - 1] + def test_var_base_to_np(self): with fluid.dygraph.guard(): var = fluid.dygraph.to_variable(self.array) -- GitLab