Fix an error message in tf.sparse_to_dense to include the possibility that...

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

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<string>();
 
-    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.

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 {

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<T>().setConstant(default_value.scalar<T>()());

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<int64>();
     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<int64>::ConstMatrix& ix_t,
-                         int64 n) const {
-    bool different = false;
-    bool bad_order = false;
+  inline Status IndexValid(const TTypes<int64>::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<T>()

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<int64> 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<string>({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<string>({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<string>({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<int64> 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<int64> new_order{1, 0, 2};
   st.Reorder<string>(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<int64>(n + 1));
     ix_t = ix_t.abs() % 1000;
     st.Reorder<string>({0, 1, 2, 3});
-    EXPECT_TRUE(st.IndicesValid());
+    TF_EXPECT_OK(st.IndicesValid());
     st.Reorder<string>({3, 2, 1, 0});
-    EXPECT_TRUE(st.IndicesValid());
+    TF_EXPECT_OK(st.IndicesValid());
     st.Reorder<string>({1, 0, 2, 3});
-    EXPECT_TRUE(st.IndicesValid());
+    TF_EXPECT_OK(st.IndicesValid());
     st.Reorder<string>({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<string>(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<string>(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<string>(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<int64> order{0, 1, 2};
 
   SparseTensor st(ix, vals, shape, order);
-  EXPECT_FALSE(st.IndicesValid());
+  EXPECT_FALSE(st.IndicesValid().ok());
 
   st.Reorder<string>(order);
-  EXPECT_TRUE(st.IndicesValid());
+  TF_EXPECT_OK(st.IndicesValid());
 
   ix_t(0, 0) = 11;
   ix.matrix<int64>() = ix_t;
   st.Reorder<string>(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<int64>() = ix_t;
   st.Reorder<string>(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<int64>() = ix_t;
   st.Reorder<string>(order);
-  EXPECT_TRUE(st.IndicesValid());
+  TF_EXPECT_OK(st.IndicesValid());
 }
 
 TEST(SparseTensorTest, SparseTensorToDenseTensor) {
@@ -436,16 +448,16 @@ TEST(SparseTensorTest, Concat) {
   std::vector<int64> order{0, 1, 2};
 
   SparseTensor st(ix, vals, shape, order);
-  EXPECT_FALSE(st.IndicesValid());
+  EXPECT_FALSE(st.IndicesValid().ok());
   st.Reorder<string>(order);
-  EXPECT_TRUE(st.IndicesValid());
+  TF_EXPECT_OK(st.IndicesValid());
 
   SparseTensor concatted = SparseTensor::Concat<string>({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<int64>();
   auto conc_vals_t = concatted.values().vec<string>();

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)