Dynamic ksize and strides with MaxPool and AvgPool #9514

yongtang · 2017-04-28T15:34:16Z

This fix tries to fix the issue raised in #4746 where ksize and strides is static (attr) with max_pool (and avg_pool).

This fix changes ksize and strides to input tensor with MaxPoolV2 and AvgPoolV2 so that it is dynamic now. This fix also deprecates MaxPool and AvgPool.

This fix fixes #4746.

tensorflow-jenkins · 2017-04-28T15:34:18Z

Can one of the admins verify this patch?

josh11b · 2017-05-01T22:21:05Z

This is fine for API review, but we'd like the Python changes to be separated into a follow-on change after 3 weeks, where you also bump the GraphDef version in https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/public/version.h . This is to accommodate our 3-week limited forward compatibility goal mentioned here:
https://www.tensorflow.org/programmers_guide/data_versions#evolving_graphdef_versions and to support eventually deleting the first versions of these ops.

yzhwang

In general a nice and clean change. Thanks! I've put some comments in your 3 commits. The major issue is to make sure that current tests in pooling_ops_test.py cover your new changes and pass.

yzhwang · 2017-05-04T21:49:46Z

tensorflow/core/kernels/pooling_ops_common.h

+    std::vector<int32> ksize = ksize_;
+    std::vector<int32> stride = stride_;
+
+    if (context->num_inputs() != 1) {


Should it be if (context->num_inputs() == 3) since we know exactly there will be 3 inputs for this case?

yzhwang · 2017-05-04T21:56:46Z

tensorflow/core/kernels/maxpooling_op.cc

@@ -250,8 +254,35 @@ class MaxPoolingGradOp : public OpKernel {
    OP_REQUIRES_OK(context, context->allocate_temp(DataTypeToEnum<int64>::v(),
                                                   tensor_out.shape(),
                                                   &tensor_out_arg_max));
+    std::vector<int32> ksize = ksize_;
+    std::vector<int32> stride = stride_;
+    if (context->num_inputs() != 3) {


Same comment here, should we use if (context->num_inputs() == 5) since we already know?

yzhwang · 2017-05-04T21:57:46Z

tensorflow/core/kernels/maxpooling_op.cc

-    PoolParameters params{context,  ksize_,      stride_,
+    std::vector<int32> ksize = ksize_;
+    std::vector<int32> stride = stride_;
+    if (context->num_inputs() != 3) {


Same comment on context->num_inputs() as other places.

yzhwang · 2017-05-04T22:30:47Z

tensorflow/core/kernels/avgpooling_op.cc

+    std::vector<int32> ksize = ksize_;
+    std::vector<int32> stride = stride_;
+
+    if (context->num_inputs() != 1) {


Same comment on context->num_inputs() elsewhere.

yzhwang · 2017-05-04T22:38:12Z

tensorflow/core/kernels/avgpooling_op.cc

-    const int depth_window = ksize_[3];
+    std::vector<int32> ksize = ksize_;
+    std::vector<int32> stride = stride_;
+    if (context->num_inputs() != 2) {


Same comment, should we use num_inputs() == 4?

Thanks @yzhwang. The PR has been updated with all related issue fixed.

yzhwang · 2017-05-04T23:06:55Z

tensorflow/python/kernel_tests/pooling_ops_test.py

@@ -1452,17 +1452,6 @@ def testOpEdgeCases(self):
      if test.is_gpu_available():
        pool_funcs.append(nn_ops.max_pool_with_argmax)
      for pool_func in pool_funcs:
-        # Illegal strides.
-        with self.assertRaisesRegexp(


Is there any specific reason to exclude this test?

yzhwang · 2017-05-04T23:41:34Z

tensorflow/python/ops/nn_grad.py

@@ -491,6 +491,19 @@ def _AvgPoolGrad(op, grad):
      data_format=op.get_attr("data_format"))


+@ops.RegisterGradient("AvgPoolV2")
+def _AvgPoolGradV2(op, grad):


We should add tests that actually take in ksizes and strides as input tensors during session running, but not like the current tests where ksizes and strides are still provided when building the graph? So that we can test whether the change works. For example, we could define ksizes and strides as variables and feed values only when we run the session.

This in general holds for all the operators you changed: MaxPool, MaxPoolGrad, MaxPoolGradGrad, AvgPool, AvgPoolGrad, AvgPoolGradGrad.

Thanks @yzhwang The PR has been updated with tests using placeholder now. Please take a look.

tensorflow-jenkins · 2017-05-11T21:15:32Z

Can one of the admins verify this patch?

rmlarsen · 2017-05-16T17:08:16Z

@yongtang any update on this? Could you please rebase to resolve conflicts?

This fix tries to fix the issue raised in 4746 where ksize is static (attr) with max_pool. This fix changes ksize to input tensor so that it is dynamic now. This fix fixes 4746. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

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

yongtang · 2017-05-18T18:28:38Z

@rmlarsen @yzhwang The PR has been rebased and updated. Please take a look.

rmlarsen · 2017-05-19T00:32:49Z

@tensorflow-jenkins test this please

rmlarsen · 2017-05-19T00:42:48Z

tensorflow/python/kernel_tests/pooling_ops_test.py

@@ -103,17 +103,18 @@ def _VerifyOneType(self, pool_func, input_sizes, ksize, strides, padding,
        t = test_util.NHWCToNCHW(t)
        ksize = test_util.NHWCToNCHW(ksize)
        strides = test_util.NHWCToNCHW(strides)
+      k = array_ops.placeholder(dtypes.int32, shape=[4])


Could you test with both placeholders and and numpy arrays?

rmlarsen · 2017-05-19T04:34:05Z

@yongtang There are a number of test failures due to the changes, please address those.

martinwicke · 2017-06-14T05:23:04Z

I'll close this, as it appears inactive. Please create a new PR if you would like to pick this back up.

hgaiser · 2017-07-26T11:25:01Z

@yongtang is this still being worked on? It appears to be exactly what I need.

yongtang · 2017-07-30T02:39:23Z

@hgaiser A new PR #11875 has been created to try to address the Jenkins test failures.

googlebot added the cla: yes label Apr 28, 2017

yongtang mentioned this pull request Apr 28, 2017

ksize in max_pool #4746

Closed

zheng-xq requested review from keveman and removed request for keveman May 1, 2017 20:09

vrv added the API review API Review label May 1, 2017

vrv requested a review from yzhwang May 1, 2017 21:04

vrv assigned yzhwang May 1, 2017

josh11b removed the API review API Review label May 1, 2017

yzhwang reviewed May 5, 2017

View reviewed changes

vrv added the stat:awaiting response Status - Awaiting response from author label May 5, 2017

yongtang added 3 commits May 18, 2017 10:43

Dynamic ksize with max_pool

33ae692

This fix tries to fix the issue raised in 4746 where ksize is static (attr) with max_pool. This fix changes ksize to input tensor so that it is dynamic now. This fix fixes 4746. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Add dynamic ksize to MaxPoolGrad and MaxPoolGradGrad.

65ec439

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

Add dynamic ksize to AvgPool, and use placeholder for testing.

1a090b4

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

yongtang force-pushed the 4746-ksize-max-pool branch from 79f5e94 to 1a090b4 Compare May 18, 2017 18:25

rmlarsen reviewed May 19, 2017

View reviewed changes

martinwicke added the stale This label marks the issue/pr stale - to be closed automatically if no activity label Jun 14, 2017

martinwicke closed this Jun 14, 2017

yongtang mentioned this pull request Jul 30, 2017

Dynamic ksize and strides with MaxPool #11875

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic ksize and strides with MaxPool and AvgPool #9514

Dynamic ksize and strides with MaxPool and AvgPool #9514

yongtang commented Apr 28, 2017

tensorflow-jenkins commented Apr 28, 2017

josh11b commented May 1, 2017

yzhwang left a comment

yzhwang May 4, 2017

yzhwang May 4, 2017

yzhwang May 4, 2017

yzhwang May 4, 2017

yzhwang May 4, 2017

yongtang May 18, 2017

yzhwang May 4, 2017

yzhwang May 4, 2017

yongtang May 18, 2017

tensorflow-jenkins commented May 11, 2017

rmlarsen commented May 16, 2017

yongtang commented May 18, 2017

rmlarsen commented May 19, 2017

rmlarsen May 19, 2017

rmlarsen commented May 19, 2017

martinwicke commented Jun 14, 2017

hgaiser commented Jul 26, 2017

yongtang commented Jul 30, 2017

Dynamic ksize and strides with MaxPool and AvgPool #9514

Dynamic ksize and strides with MaxPool and AvgPool #9514

Conversation

yongtang commented Apr 28, 2017

tensorflow-jenkins commented Apr 28, 2017

josh11b commented May 1, 2017

yzhwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tensorflow-jenkins commented May 11, 2017

rmlarsen commented May 16, 2017

yongtang commented May 18, 2017

rmlarsen commented May 19, 2017

Choose a reason for hiding this comment

rmlarsen commented May 19, 2017

martinwicke commented Jun 14, 2017

hgaiser commented Jul 26, 2017

yongtang commented Jul 30, 2017