From 73d557cc88ee86834917088416e5ce8783d11798 Mon Sep 17 00:00:00 2001 From: "A. Unique TensorFlower" Date: Wed, 24 Feb 2016 20:01:21 -0800 Subject: [PATCH] Fix an error message in tf.sparse_to_dense to include the possibility that indices are invalid because they are out of bounds. Change: 115522264 --- .../core/kernels/serialize_sparse_op.cc | 5 +- tensorflow/core/kernels/sparse_reorder_op.cc | 2 +- tensorflow/core/kernels/sparse_to_dense_op.cc | 5 +- tensorflow/core/util/sparse/sparse_tensor.h | 37 ++++++++----- .../core/util/sparse/sparse_tensor_test.cc | 54 +++++++++++-------- .../sparse_to_dense_op_py_test.py | 33 ++++++++++-- 6 files changed, 89 insertions(+), 47 deletions(-) diff --git a/tensorflow/core/kernels/serialize_sparse_op.cc b/tensorflow/core/kernels/serialize_sparse_op.cc index dbc60c264b5..9729beeddaa 100644 --- a/tensorflow/core/kernels/serialize_sparse_op.cc +++ b/tensorflow/core/kernels/serialize_sparse_op.cc @@ -129,10 +129,7 @@ class SerializeManySparseOp : public OpKernel { Tensor serialized_sparse(DT_STRING, TensorShape({N, 3})); auto serialized_sparse_t = serialized_sparse.matrix(); - OP_REQUIRES(context, input_st.IndicesValid(), - errors::InvalidArgument("Input SparseTensor fails check for " - "lexicographic ordering of indices. " - "Cannot split.")); + OP_REQUIRES_OK(context, input_st.IndicesValid()); // We can generate the output shape proto string now, for all // minibatch entries. diff --git a/tensorflow/core/kernels/sparse_reorder_op.cc b/tensorflow/core/kernels/sparse_reorder_op.cc index 7d42af0b7c3..baf7df70ea8 100644 --- a/tensorflow/core/kernels/sparse_reorder_op.cc +++ b/tensorflow/core/kernels/sparse_reorder_op.cc @@ -62,7 +62,7 @@ class SparseReorderOp : public OpKernel { // Check if the sparse tensor is already ordered correctly sparse::SparseTensor input_sp(input_ind, input_val, input_shape, std_order); - if (input_sp.IndicesValid()) { + if (input_sp.IndicesValid().ok()) { context->set_output(0, input_sp.indices()); context->set_output(1, input_sp.values()); } else { diff --git a/tensorflow/core/kernels/sparse_to_dense_op.cc b/tensorflow/core/kernels/sparse_to_dense_op.cc index e4e731d69ce..e60419d9fc2 100644 --- a/tensorflow/core/kernels/sparse_to_dense_op.cc +++ b/tensorflow/core/kernels/sparse_to_dense_op.cc @@ -121,10 +121,7 @@ class SparseToDense : public OpKernel { order); if (validate_indices_) { - OP_REQUIRES(c, st.IndicesValid(), - errors::InvalidArgument("Indices are not valid: not " - "lexicographically sorted or " - "containing repeats.")); + OP_REQUIRES_OK(c, st.IndicesValid()); } output->flat().setConstant(default_value.scalar()()); diff --git a/tensorflow/core/util/sparse/sparse_tensor.h b/tensorflow/core/util/sparse/sparse_tensor.h index 67555b454fc..309e8c0c5b4 100644 --- a/tensorflow/core/util/sparse/sparse_tensor.h +++ b/tensorflow/core/util/sparse/sparse_tensor.h @@ -68,18 +68,21 @@ class SparseTensor { DataType dtype() const { return vals_.dtype(); } - bool IndicesValid() const { + Status IndicesValid() const { const auto ix_t = ix_.matrix(); for (int64 ord : order_) { - CHECK_GE(ord, 0) << "Order was not provided. Provide an order at " - "construction time or run ReorderInPlace"; + if (ord < 0) { + return errors::FailedPrecondition( + "Order was not provided. Provide an order at " + "construction time or run ReorderInPlace"); + } } for (std::size_t n = 0; n < num_entries(); ++n) { - if (!IndexValid(ix_t, n)) return false; + TF_RETURN_IF_ERROR(IndexValid(ix_t, n)); } - return true; + return Status::OK(); } // Returns the tensor shape (the dimensions of the "densified" @@ -154,11 +157,11 @@ class SparseTensor { } // Helper for IndicesValid() - inline bool IndexValid(const TTypes::ConstMatrix& ix_t, - int64 n) const { - bool different = false; - bool bad_order = false; + inline Status IndexValid(const TTypes::ConstMatrix& ix_t, + int n) const { bool valid = true; + bool different = false; + bool increasing = true; if (n == 0) { for (int di = 0; di < dims_; ++di) { if (ix_t(n, di) < 0 || ix_t(n, di) >= shape_.dim_size(di)) @@ -171,13 +174,19 @@ class SparseTensor { valid = false; int64 diff = ix_t(n, order_[di]) - ix_t(n - 1, order_[di]); if (diff > 0) different = true; - if (!different && diff < 0) bad_order = true; + if (!different && diff < 0) increasing = false; } } - if (!valid) return false; // Out of bounds - if (!different) return false; // The past two indices are identical... - if (bad_order) return false; // Decreasing in order. - return true; + if (!valid) { + return errors::InvalidArgument("Index ", n, " is out of bounds."); + } + if (!increasing) { + return errors::InvalidArgument("Index ", n, " is out of order."); + } + if (!different) { + return errors::InvalidArgument("Index ", n, " is repeated."); + } + return Status::OK(); } // Helper for ToDense() diff --git a/tensorflow/core/util/sparse/sparse_tensor_test.cc b/tensorflow/core/util/sparse/sparse_tensor_test.cc index 70a4ee4f938..e4881cfcc80 100644 --- a/tensorflow/core/util/sparse/sparse_tensor_test.cc +++ b/tensorflow/core/util/sparse/sparse_tensor_test.cc @@ -21,6 +21,7 @@ limitations under the License. #include "third_party/eigen3/unsupported/Eigen/CXX11/Tensor" #include "tensorflow/core/framework/tensor.h" #include "tensorflow/core/framework/tensor_types.h" +#include "tensorflow/core/lib/core/status_test_util.h" #include "tensorflow/core/lib/strings/str_util.h" #include "tensorflow/core/platform/test.h" @@ -100,12 +101,14 @@ TEST(SparseTensorTest, SparseTensorConstruction) { TensorShape shape({10, 10, 10}); std::vector order{0, 1, 2}; SparseTensor st(ix, vals, shape, order); - EXPECT_FALSE(st.IndicesValid()); // Out of order + Status st_indices_valid = st.IndicesValid(); + EXPECT_FALSE(st_indices_valid.ok()); + EXPECT_EQ("Index 2 is out of order.", st_indices_valid.error_message()); // Regardless of how order is updated; so long as there are no // duplicates, the resulting indices are valid. st.Reorder({2, 0, 1}); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); EXPECT_EQ(vals_t(0), "hi0"); EXPECT_EQ(vals_t(1), "hi3"); EXPECT_EQ(vals_t(2), "hi2"); @@ -115,7 +118,7 @@ TEST(SparseTensorTest, SparseTensorConstruction) { ix_t = ix_c; vals_t = vals_c; st.Reorder({0, 1, 2}); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); EXPECT_EQ(vals_t(0), "hi0"); EXPECT_EQ(vals_t(1), "hi4"); EXPECT_EQ(vals_t(2), "hi3"); @@ -125,7 +128,7 @@ TEST(SparseTensorTest, SparseTensorConstruction) { ix_t = ix_c; vals_t = vals_c; st.Reorder({2, 1, 0}); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); } TEST(SparseTensorTest, EmptySparseTensorAllowed) { @@ -138,12 +141,12 @@ TEST(SparseTensorTest, EmptySparseTensorAllowed) { TensorShape shape({10, 10, 10}); std::vector order{0, 1, 2}; SparseTensor st(ix, vals, shape, order); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); EXPECT_EQ(st.order(), order); std::vector new_order{1, 0, 2}; st.Reorder(new_order); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); EXPECT_EQ(st.order(), new_order); } @@ -162,13 +165,13 @@ TEST(SparseTensorTest, SortingWorksCorrectly) { ix_t = ix_t.random(Eigen::internal::UniformRandomGenerator(n + 1)); ix_t = ix_t.abs() % 1000; st.Reorder({0, 1, 2, 3}); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); st.Reorder({3, 2, 1, 0}); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); st.Reorder({1, 0, 2, 3}); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); st.Reorder({3, 0, 2, 1}); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); } } @@ -196,17 +199,21 @@ TEST(SparseTensorTest, ValidateIndicesFindsInvalid) { SparseTensor st(ix, vals, shape, order); st.Reorder(order); - EXPECT_FALSE(st.IndicesValid()); // two indices are identical + Status st_indices_valid = st.IndicesValid(); + EXPECT_FALSE(st_indices_valid.ok()); + EXPECT_EQ("Index 1 is repeated.", st_indices_valid.error_message()); ix_orig(1, 2) = 1; ix_t = ix_orig; st.Reorder(order); - EXPECT_TRUE(st.IndicesValid()); // second index now (0, 0, 1) + TF_EXPECT_OK(st.IndicesValid()); // second index now (0, 0, 1) ix_orig(0, 2) = 1; ix_t = ix_orig; st.Reorder(order); - EXPECT_FALSE(st.IndicesValid()); // first index now (0, 0, 1) + st_indices_valid = st.IndicesValid(); + EXPECT_FALSE(st_indices_valid.ok()); // first index now (0, 0, 1) + EXPECT_EQ("Index 1 is repeated.", st_indices_valid.error_message()); } TEST(SparseTensorTest, SparseTensorCheckBoundaries) { @@ -224,25 +231,30 @@ TEST(SparseTensorTest, SparseTensorCheckBoundaries) { std::vector order{0, 1, 2}; SparseTensor st(ix, vals, shape, order); - EXPECT_FALSE(st.IndicesValid()); + EXPECT_FALSE(st.IndicesValid().ok()); st.Reorder(order); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); ix_t(0, 0) = 11; ix.matrix() = ix_t; st.Reorder(order); - EXPECT_FALSE(st.IndicesValid()); + Status st_indices_valid = st.IndicesValid(); + EXPECT_FALSE(st_indices_valid.ok()); + // Error message references index 4 because of the call to Reorder. + EXPECT_EQ("Index 4 is out of bounds.", st_indices_valid.error_message()); ix_t(0, 0) = -1; ix.matrix() = ix_t; st.Reorder(order); - EXPECT_FALSE(st.IndicesValid()); + st_indices_valid = st.IndicesValid(); + EXPECT_FALSE(st_indices_valid.ok()); + EXPECT_EQ("Index 0 is out of bounds.", st_indices_valid.error_message()); ix_t(0, 0) = 0; ix.matrix() = ix_t; st.Reorder(order); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); } TEST(SparseTensorTest, SparseTensorToDenseTensor) { @@ -436,16 +448,16 @@ TEST(SparseTensorTest, Concat) { std::vector order{0, 1, 2}; SparseTensor st(ix, vals, shape, order); - EXPECT_FALSE(st.IndicesValid()); + EXPECT_FALSE(st.IndicesValid().ok()); st.Reorder(order); - EXPECT_TRUE(st.IndicesValid()); + TF_EXPECT_OK(st.IndicesValid()); SparseTensor concatted = SparseTensor::Concat({st, st, st, st}); EXPECT_EQ(concatted.order(), st.order()); TensorShape expected_shape({40, 10, 10}); EXPECT_EQ(concatted.shape(), expected_shape); EXPECT_EQ(concatted.num_entries(), 4 * N); - EXPECT_TRUE(concatted.IndicesValid()); + TF_EXPECT_OK(concatted.IndicesValid()); auto conc_ix_t = concatted.indices().matrix(); auto conc_vals_t = concatted.values().vec(); diff --git a/tensorflow/python/kernel_tests/sparse_to_dense_op_py_test.py b/tensorflow/python/kernel_tests/sparse_to_dense_op_py_test.py index 35fdbbe5e4f..0a3119bf231 100644 --- a/tensorflow/python/kernel_tests/sparse_to_dense_op_py_test.py +++ b/tensorflow/python/kernel_tests/sparse_to_dense_op_py_test.py @@ -111,13 +111,27 @@ class SparseToDenseTest(tf.test.TestCase): with self.assertRaisesOpError("default_value should be a scalar"): dense.eval() - def testInvalidIndicesWithWithoutValidation(self): + def testOutOfBoundsIndicesWithWithoutValidation(self): + with self.test_session(): + dense = _SparseToDense( + sparse_indices=[[1], [10]], output_size=[5], + sparse_values=[-1.0, 1.0], default_value=0.0) + with self.assertRaisesOpError("Index 1 is out of bounds."): + dense.eval() + # Disable checks, the allocation should still fail. + with self.assertRaisesOpError("out of bounds"): + dense_without_validation = _SparseToDense( + sparse_indices=[[1], [10]], output_size=[5], + sparse_values=[-1.0, 1.0], default_value=0.0, + validate_indices=False) + dense_without_validation.eval() + + def testRepeatingIndicesWithWithoutValidation(self): with self.test_session(): dense = _SparseToDense( sparse_indices=[[1], [1]], output_size=[5], sparse_values=[-1.0, 1.0], default_value=0.0) - with self.assertRaisesOpError( - "not lexicographically sorted or containing repeats"): + with self.assertRaisesOpError("Index 1 is repeated."): dense.eval() # Disable checks dense_without_validation = _SparseToDense( @@ -125,6 +139,19 @@ class SparseToDenseTest(tf.test.TestCase): sparse_values=[-1.0, 1.0], default_value=0.0, validate_indices=False) dense_without_validation.eval() + def testUnsortedIndicesWithWithoutValidation(self): + with self.test_session(): + dense = _SparseToDense( + sparse_indices=[[2], [1]], output_size=[5], + sparse_values=[-1.0, 1.0], default_value=0.0) + with self.assertRaisesOpError("Index 1 is out of order."): + dense.eval() + # Disable checks + dense_without_validation = _SparseToDense( + sparse_indices=[[2], [1]], output_size=[5], + sparse_values=[-1.0, 1.0], default_value=0.0, validate_indices=False) + dense_without_validation.eval() + def testShapeInferenceKnownShape(self): with self.test_session(use_gpu=False): indices = tf.placeholder(tf.int64) -- GitLab