Add support for sparse_reduce_max and sparse_reduce_max_sparse #10056

yongtang · 2017-05-20T02:11:09Z

This commit tries to address the issue raised in #10002 to have
the support for sparse_reduce_max and sparse_reduce_max_sparse.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

tensorflow-jenkins · 2017-05-20T02:11:10Z

Can one of the admins verify this patch?

concretevitamin · 2017-05-22T16:56:37Z

@yongtang I was imagining the code for sparse_reduce_sum_op.cc and sparse_reduce_max_op.cc can be mostly shared. A remotely related example is the ScatterUpdateOp, which has a template param on UpdateOp (ADD, etc.).

yongtang · 2017-05-24T15:06:33Z

Thanks @concretevitamin for the review. The PR has been updated so that sparse_reduce_sum and sparse_reduce_max share the same code base. Please take a look.

concretevitamin

This looks great! Left a few comments.

concretevitamin · 2017-05-24T21:24:35Z

tensorflow/core/kernels/sparse_reduce_op.cc

@@ -195,7 +209,7 @@ class SparseReduceSumOp : public OpKernel {
    // g.group() provides the coordinates of a particular reduced value.
    sp.Reorder<T>(reduction.reorder_dims);
    for (const auto &g : sp.group(reduction.group_by_dims)) {
-      group_sum.device(ctx->eigen_cpu_device()) = g.template values<T>().sum();
+      Op::template Run<T>(ctx, group_sum, g.template values<T>());


Rename group_sum? Something like reduced_val or something better.

Thanks @concretevitamin. The PR has been updated.

concretevitamin · 2017-05-24T21:25:50Z

tensorflow/core/kernels/sparse_reduce_op.cc

@@ -265,7 +286,7 @@ class SparseReduceSumSparseOp : public OpKernel {
    auto group_sum = tmp_group_sum.scalar<T>();
    int64 i = 0;
    for (const auto &g : sp.group(reduction.group_by_dims)) {
-      group_sum.device(ctx->eigen_cpu_device()) = g.template values<T>().sum();
+      Op::template Run<T>(ctx, group_sum, g.template values<T>());


concretevitamin · 2017-05-24T21:31:23Z

tensorflow/core/kernels/sparse_reduce_max_op_test.cc

@@ -0,0 +1,125 @@
+/* Copyright 2016 The TensorFlow Authors. All Rights Reserved.


I'd prefer if you could leave out this file. The reason we originally had a C++ test file for SparseReduceSum was to ease debugging. Since now most of the kernels share the same logic, this is no longer needed. The functional correctness of the new op can be covered by the more lightweight Python unit tests.

Thanks. Done.

concretevitamin · 2017-05-24T21:34:24Z

tensorflow/python/kernel_tests/sparse_ops_test.py

@@ -510,10 +510,86 @@ def testNoEmptyRows(self):
      self.assertAllEqual(empty_row_indicator_out, np.zeros(2).astype(np.bool))


+class SparseReduceMaxTest(test_util.TensorFlowTestCase):


Can you refactor the existing SparseReduceSumTest to plumb through an extra bool argument, such as do_sum == True / False, so that we can share most of the existing test logic?

Thanks. Done.

yongtang · 2017-05-25T03:48:47Z

@concretevitamin Thanks for the review. The PR has been updated. Please take.a look.

concretevitamin

Great! Two final nits. Please address but I'm approving first ;)

concretevitamin · 2017-05-25T18:42:50Z

tensorflow/core/kernels/sparse_reduce_op.cc

+  static void Run(OpKernelContext *ctx, typename TTypes<T>::Scalar &s, const typename TTypes<T>::UnalignedVec &v) {
+      s.device(ctx->eigen_cpu_device()) = v.sum();
+  }
+  static const char * Name() {


Let's do

StringPiece Name() { return "sum"; }

which is more succinct (and potentially safer).

concretevitamin · 2017-05-25T18:43:19Z

tensorflow/core/kernels/sparse_reduce_op.cc

+  static void Run(OpKernelContext *ctx, typename TTypes<T>::Scalar &s, const typename TTypes<T>::UnalignedVec &v) {
+      s.device(ctx->eigen_cpu_device()) = v.maximum();
+  }
+  static const char * Name() {


yongtang · 2017-05-30T02:40:49Z

Thanks @concretevitamin. The PR has been updated.

concretevitamin · 2017-05-30T17:25:02Z

Jenkins, test this please.

@andrewharp feel free to merge.

jhseu · 2017-06-07T00:01:57Z

This change will require API review.

josh11b · 2017-06-12T23:15:21Z

Fine for API review.

martinwicke · 2017-06-14T04:11:11Z

Can you rebase and fix the conflicts? I cannot fix things in this PR.

yongtang · 2017-06-14T17:16:11Z

Thanks @martinwicke. The PR has been rebased. Please take a look

martinwicke · 2017-06-15T15:21:56Z

Jenkins, test this please.

martinwicke · 2017-06-16T19:04:26Z

Can you please run the API goldens updater as described in the error message of the failing Linux-CPU test?

This commit tries to address the issue raised in 10002 to have the support for sparse_reduce_max and sparse_reduce_max_sparse. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

This commit moves sparse_reduce_sum_op.cc to sparse_reduce_op.cc so that both `sparse_reduce_sum` and `sparse_reduce_max` share the same code base. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>