[AVG POOL] Asymmetric padding (SAME) support. by srkreddy1238 · Pull Request #1346 · apache/tvm

srkreddy1238 · 2018-06-27T18:36:17Z

Ref. https://discuss.tvm.ai/t/avgpool-same-padding/314

srkreddy1238 · 2018-06-28T05:48:00Z

@tqchen & @Huyuwei Please have a look at this patch to support asymmetric padding with count_include_pad option.
padding attribute extended to accept 4 values by keeping 2 value logic intact.

Now the SAME padding option work perfectly for all operations.
Convolution : inserting pad operator before with pad_value = 0.
Maxpool : inserting pad operator before with pad_value=-inf
Avgpool : padding accepts (pad_top, pad_left, pad_bottom, pad_right)

FrozenGene · 2018-06-28T07:59:29Z

Do you want to continue to support Convolution / Maxpool and so on for SAME padding to accept 4 values? I have implemented it in my codebase, but my company has its own open source rule and need long time to contribute. So if you want to implement continually, I am glad to see it. And you also need to update WORKLOADS. i.e.:

_Workload = namedtuple('Workload',
                       ['in_dtype', 'out_dtype', 'height', 'width', 'channel', 'multiplier',
                        'hkernel', 'wkernel', 'hpad', 'wpad', 'hstride', 'wstride'])

Here, it only has hpad / wpad，you should update to 4 elements. It also affects schedule. For example:

    HPAD, WPAD = wkl.hpad, wkl.wpad  # You should update
    HSTR, WSTR = wkl.hstride, wkl.wstride

    VH, VW = sch.vh, sch.vw
    BC = sch.bc
    VC = sch.vc

   # You should update
    TH = H + 2*HPAD 
    TW = W + 2*WPAD

If you would like to do it, I would like to do code review it. Because I have implemented and work perfectly.

srkreddy1238 · 2018-06-28T08:40:34Z

@FrozenGene BTW, Convolution and Maxpool need not require these changes as there is alternative to achieve the same.

FrozenGene · 2018-06-28T10:10:03Z

You mean insert the pad operator? I think that is not an elegant solution. As @Huyuwei comment in this PR : dmlc/nnvm#472 this pad operator brings overhead. Extend it will make us convert much easier and clearer.

srkreddy1238 · 2018-06-28T10:22:38Z

I am open to extend the padding internal to Conv and Maxpool as well.

In any case there is no extra_pad in SAME option. We either insert pad at frontend (pad values will be [0,0] for actual operation) or we pass the pad values to the operation and the compute will add pad operation accordingly.

This PR is just accommodate count_include_pad effect with asymmetric padding (SAME).

srkreddy1238 · 2018-06-29T04:41:25Z

@Huyuwei & @tqchen any comment here ?

Huyuwei · 2018-06-29T05:24:32Z

I think it's good to support 4 pad values for pool, both max and avg. conv2d may be left for next step after further discussion, since it requires a lot of change to the code as @FrozenGene commented.

@srkreddy1238 I will give detailed comments on this PR later this weekend.

srkreddy1238 · 2018-07-02T15:11:20Z

@Huyuwei can you have a look at this ?

Huyuwei · 2018-07-02T18:09:18Z

Here is my suggestion:

On topi implementation level, always pass 4 pad values. It can be in the format of [top, left, bottom, right] or [[top, bottom], [left, right]]
On nnvm symbol level, support api like this: https://keras.io/layers/convolutional/#zeropadding2d
Support asymmetric padding for both avg and max pooling, to keep these two kinds of pooling consistent.

* topi : always accespt 4 padding values as (top, left, bottom, right) * nnvm accepts 1 / 2 / 4 padding inputs similar to keras. * Support both Max and Avg.

…symmetric padding.

srkreddy1238 · 2018-07-03T06:24:06Z

@Huyuwei & @FrozenGene have a look at the new changes.

topi pad format fixed as [top, left, bottom, right]
nnvm accepts padding as (int) or (int, int) or (int, int, int, int)
both maxpool and avgpool carries the same syntax for padding.

Huyuwei · 2018-07-04T00:47:05Z

+                "Padding support both symmetric and asymmetric as"
+                "one int : same padding used on all sides"
+                "two int : bottom, right will use same padding as top, left"
+                "four int : padding just as given");


four int : padding width in the order of top, left, bottom, right

Huyuwei · 2018-07-04T00:47:20Z

+                "Padding support both symmetric and asymmetric as"
+                "one int : same padding used on all sides"
+                "two int : bottom, right will use same padding as top, left"
+                "four int : padding just as given");


four int : padding width in the order of top, left, bottom, right

Huyuwei · 2018-07-04T00:53:44Z

+    pad[3] = pad[0];
+  } else if (param.padding.ndim() == 2) {
+    pad[0] *= 2;
+    pad[1] *= 2;


better to use pad_h pad_w instead of pad[0] pad[1]

pad_h = pad[0] * 2 pad_w = pad[1] * 2

Huyuwei · 2018-07-04T00:58:05Z

+  if (param.padding.ndim() == 1) {
+    pad[1] = pad[0];
+    pad[2] = pad[0];
+    pad[3] = pad[0];


the logic here seems not correct, it should be:

pad_h = pad[0] * 2 pad_w = pad[0] * 2

Good catch, thanks.

srkreddy1238 · 2018-07-04T03:53:04Z

@Huyuwei review comments handled.

Huyuwei · 2018-07-04T04:25:10Z

  CHECK_EQ(kernel_size.size(), 2) << "Pooling kernel_size must have 2 elements";
  CHECK_EQ(stride_size.size(), 2) << "Pooling stride_size must have 2 elements";
-  CHECK_EQ(padding_size.size(), 2) << "Pooling padding_size must have 2 elements";
+  CHECK((padding_size.size() == 4)) << "Pooling padding_size must have 4 elements";


use CHECK_EQ

Huyuwei · 2018-07-04T04:34:59Z

+        for pooling_type in ["MAX", "AVG"]:
+            for input_shape in [[2, 9, 10, 2], [2, 10, 9, 2]]:
+                for window_shape in [[1, 1], [2, 1], [2, 3]]:
+                    if padding != "SAME":


the testing script is not clean. I think it's ok to test a few important cases using the old style.

srkreddy1238 · 2018-07-04T05:17:06Z

@Huyuwei patched as per review. Please check.

srkreddy1238 · 2018-07-04T14:48:07Z

@Huyuwei thanks for the review.

srkreddy1238 force-pushed the master branch 3 times, most recently from 5015a53 to 9d94df6 Compare June 28, 2018 05:13

srkreddy1238 force-pushed the master branch from 9d94df6 to 621dca9 Compare July 2, 2018 05:10

srkreddy1238 added 2 commits July 2, 2018 23:05

[AVG POOL] Asymmetric padding (SAME) support.

b75e5ba

[AVG POOL] typo fix

690ca55

srkreddy1238 force-pushed the master branch from 834c44f to 690ca55 Compare July 2, 2018 17:35

tqchen added status: review in progress status: need update need update based on feedbacks labels Jul 3, 2018

srkreddy1238 added 3 commits July 3, 2018 10:37

[POOL] Below changes as per the discussion.

652f778

* topi : always accespt 4 padding values as (top, left, bottom, right) * nnvm accepts 1 / 2 / 4 padding inputs similar to keras. * Support both Max and Avg.

[NNVM][TENSORFLOW] external pad op removed as the pool now supports a…

4ff8386

…symmetric padding.

[POOL] Keep 2 ints as default for pool operators.

efb0c7e

Huyuwei requested changes Jul 4, 2018

View reviewed changes

[POOL] Review comments addressed.

ec831ae

Huyuwei reviewed Jul 4, 2018

View reviewed changes

[POOL] Review comments (1)

ebe7895

Huyuwei approved these changes Jul 4, 2018

View reviewed changes

tqchen added status: accepted and removed status: need update need update based on feedbacks status: review in progress labels Jul 4, 2018

tqchen merged commit 5f9c63a into apache:master Jul 4, 2018

tqchen pushed a commit to tqchen/tvm that referenced this pull request Jul 6, 2018

[AVG POOL] Asymmetric padding (SAME) support. (apache#1346)

f9cb969

mnuyens pushed a commit to mnuyens/tvm that referenced this pull request Jul 10, 2018

[AVG POOL] Asymmetric padding (SAME) support. (apache#1346)

30f09be

sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018

[AVG POOL] Asymmetric padding (SAME) support. (apache#1346)

32a28f1

Rasterer mentioned this pull request Feb 27, 2019

[RFC] Asymmetric padding for convolution #2682

Closed

3 tasks

Conversation

srkreddy1238 commented Jun 27, 2018

Uh oh!

srkreddy1238 commented Jun 28, 2018

Uh oh!

FrozenGene commented Jun 28, 2018

Uh oh!

srkreddy1238 commented Jun 28, 2018

Uh oh!

FrozenGene commented Jun 28, 2018

Uh oh!

srkreddy1238 commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srkreddy1238 commented Jun 29, 2018

Uh oh!

Huyuwei commented Jun 29, 2018

Uh oh!

srkreddy1238 commented Jul 2, 2018

Uh oh!

Huyuwei commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srkreddy1238 commented Jul 3, 2018

Uh oh!

Huyuwei Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

Huyuwei Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

Huyuwei Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

Huyuwei Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

srkreddy1238 Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

srkreddy1238 commented Jul 4, 2018

Uh oh!

Huyuwei Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

Huyuwei Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

srkreddy1238 commented Jul 4, 2018

Uh oh!

srkreddy1238 commented Jul 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

srkreddy1238 commented Jun 28, 2018 •

edited

Loading

Huyuwei commented Jul 2, 2018 •

edited

Loading