Add an optional switch to control edge behavior of average pooling #6303

oscarriddle · 2018-03-20T01:58:14Z

Because the commits on old thread #6296 got messed up, I try to create this new thread based on a clean branch. Hi @Noiredd, would you please help close that thread, it will be very helpful.

Add an optional boolean switch named "dynamic_ave_pool_size" in the caffe.proto, and default=false.
Pass the handle of layer_param_.pooling_param() into Forward_cpu() and Backward_cpu() functions in pooling_layer.cpp, and the AvePoolForward() function in pooling_layer.cu (The AvePoolBackward() seems correct already).
If the dynamic_ave_pool_size==true, the variable "pool_size" would be updated immediately after the 4 coordinates been updated.

I just tested the Forward_cpu() version, it seems good. I will test the GPU forward version later on, but need some help on the Backward_cpu().

Thanks,
Xiaolun Cao

Noiredd

The only thing this PR is missing is a test case for this functionality. As it is, it passes the tests because the default value of the flag is false, so it's disabled and never tested.

Also, I left a couple minor comments in the review.

Noiredd · 2018-03-27T07:29:54Z

src/caffe/layers/pooling_layer.cpp

@@ -127,6 +127,7 @@ void PoolingLayer<Dtype>::Reshape(const vector<Blob<Dtype>*>& bottom,
 template <typename Dtype>
 void PoolingLayer<Dtype>::Forward_cpu(const vector<Blob<Dtype>*>& bottom,
      const vector<Blob<Dtype>*>& top) {
+  PoolingParameter pool_param = this->layer_param_.pooling_param();


How about, instead of reading the param every time, just add a class member variable to hold the value of the flag, and just set it once during LayerSetUp?

Noiredd · 2018-03-27T07:31:10Z

src/caffe/layers/pooling_layer.cpp

@@ -203,6 +204,9 @@ void PoolingLayer<Dtype>::Forward_cpu(const vector<Blob<Dtype>*>& bottom,
            wstart = max(wstart, 0);
            hend = min(hend, height_);
            wend = min(wend, width_);
+            if (pool_param.dynamic_ave_pool_size() == true) {


if (pool_param.dynamic_ave_pool_size()) will suffice. And if you decide to follow my advice from the comment above, it will get even simpler (if (member_var_)).

Noiredd · 2018-03-27T07:32:18Z

src/caffe/layers/pooling_layer.cu

    hstart = max(hstart, 0);
    wstart = max(wstart, 0);
    hend = min(hend, height);
    wend = min(wend, width);
+    if (dynamic_ave_pool_size == true) {


Like above: if (dynamic_ave_pool_size) would be cleaner.

Noiredd · 2018-03-27T07:41:45Z

src/caffe/proto/caffe.proto

+  // If dynamic_ave_pool_size is set to true, every iteration of AVE pooling will only
+  // concerns valid elements: (sum of elements within kernel)/(valid elements num)
+  // Otherwise, invalid padded elements will always be counted in the denominator
+  optional bool dynamic_ave_pool_size = 13 [default = false];


My last comment and the most opinion-based: is this the best name for this flag? How about valid_ave_pooling?
Also, while it's good that you've explained what this does, I think that the comment should make it more clear that valid pooling ignores the padding elements in the calculation.

oscarriddle · 2018-03-27T09:56:15Z

Hi Noiredd,

Thanks for help reviewing my fix. Now the code has been updated accordingly.
However, I'm not quite sure about why the version 1fddd65 can pass the travis CI, but after I modified several comment lines, and new version 14f04c9 and ad291f5 become failed. I'm still trying to address the issue.
The new fix is test passed under my project. I think I can help write a caffe test for this if you think that would be much better.

Thanks,
Xiaolun Cao

oscarriddle · 2018-03-27T10:11:39Z

E: Failed to fetch http://dl.bintray.com/apache/cassandra/dists/39x/main/binary-amd64/Packages 502 Bad Gateway [IP: 52.10.151.113 80] E: Failed to fetch http://dl.bintray.com/apache/cassandra/dists/39x/main/binary-i386/Packages 502 Bad Gateway [IP: 52.10.151.113 80] E: Some index files failed to download. They have been ignored, or old ones used instead.

Seems something happened to Travis CI server, perhaps I need to try again later.

Noiredd · 2018-03-27T12:05:14Z

Thanks for the fixes. One other thing: now that you use a member variable, we don't need to declare PoolingParameter pool_param in forward/backward methods.

As I said, this passes tests because it is actually never tested - every time any test case instantiates a Pooling Layer, it does so with its default setting, which is without valid_ave_pooling. For this reason this needs an explicit test for this particular functionality.

Those random Travis errors are obviously not related to the code, so let's not worry about them for now. If need be, I will restart the tests.

oscarriddle · 2018-03-28T02:35:44Z

The param declaration within Forward/Backward are removed now, and new ver. passed the travis.

Yes, I suggest this needs specific tests, and I'm looking into the existed tests and learning to create one. Besides, I think this feature shall be both tested under the CPU and GPU backend.

Thanks,

oscarriddle · 2018-03-28T03:37:33Z

I created an explicit test named "TestForwardAveValid", it will turn on the member variable valid_ave_pooling and compare the outputs.

I noticed the comments in the cuDNN version tests, says cuDNN doesn't support padding. So I'm not quite sure about the test method in cuDNN version.

If you would help take a review, it'll be much appreciated.

Thanks,

Noiredd · 2018-04-19T07:38:13Z

cuDNN doesn't support padding

Must be some old information, I'm quite sure we can use padded pooling with cuDNN.
I think we should test the padded version too, as without it there is no difference between valid/invalid pooling.

Another thing is that the blob that you're using is quite trivial - filled with a constant value. Are you sure this will be a solid test?
Also, what do you think about testing the backward pass on this as well?

oscarriddle · 2018-07-03T11:04:12Z

Sorry, I was busy working on other projects afterwards.

The test blob that used in this test is imitating the existed average pool test, which is "TestForwardAve".

I'm not 100% sure about the meaning of gradient test that in test_pooling_layer.cpp, like "TestGradientAve" corresponds to "TestForwardAve".

I think my previous code modification shall be safe enough, however add a gradient test would also be a good idea. Would you help check the actual mechanism and sufficiency of "TestGradientAve".

Thanks,

oscarriddle added 6 commits March 19, 2018 22:57

fix ave pool_size bug by using new switch param dynamic_ave_pool_size

e82ba22

modify gpu version, remove this from static function, remove tab

73b94a5

update boolean identifier to bool

1d58db8

fix cuda compile error

dcc6bcc

fix code style errors

480ccf3

Add comment in caffe.proto

b504f3d

Noiredd mentioned this pull request Mar 20, 2018

Average pooling with padding - behavior on edges #6296

Closed

Noiredd changed the title ~~[AVE POOL ISSUE] New thread of #6296, Add an optional switch to support dynamic pool_size in AVE pooling~~ Add an optional switch to control edge behavior of average pooling Mar 20, 2018

Noiredd added enhancement in progress labels Mar 20, 2018

Noiredd requested changes Mar 27, 2018

View reviewed changes

oscarriddle added 3 commits March 27, 2018 16:31

imporve fixe code according to Noiredd

1fddd65

change the comment of param valid_ave_pooling to be more clear

14f04c9

travis ci failed, unknown reason

ad291f5

remove redundant layer_param declaration within Forward/Backward Methods

6cf6819

Add a test for valid ave pooling, named TestForwardAveValid

c547686

oscarriddle added 3 commits March 28, 2018 11:57

fix code style related issue

82e63c2

fix code style related issue, whitespaces

ee9ea41

fix code style related issue, whitespaces

3a1b805

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an optional switch to control edge behavior of average pooling #6303

Add an optional switch to control edge behavior of average pooling #6303

oscarriddle commented Mar 20, 2018 •

edited

Noiredd left a comment

Noiredd Mar 27, 2018

Noiredd Mar 27, 2018

Noiredd Mar 27, 2018

Noiredd Mar 27, 2018

oscarriddle commented Mar 27, 2018

oscarriddle commented Mar 27, 2018

Noiredd commented Mar 27, 2018

oscarriddle commented Mar 28, 2018

oscarriddle commented Mar 28, 2018

Noiredd commented Apr 19, 2018

oscarriddle commented Jul 3, 2018

Add an optional switch to control edge behavior of average pooling #6303

Are you sure you want to change the base?

Add an optional switch to control edge behavior of average pooling #6303

Conversation

oscarriddle commented Mar 20, 2018 • edited

Noiredd left a comment

Choose a reason for hiding this comment

Noiredd Mar 27, 2018

Choose a reason for hiding this comment

Noiredd Mar 27, 2018

Choose a reason for hiding this comment

Noiredd Mar 27, 2018

Choose a reason for hiding this comment

Noiredd Mar 27, 2018

Choose a reason for hiding this comment

oscarriddle commented Mar 27, 2018

oscarriddle commented Mar 27, 2018

Noiredd commented Mar 27, 2018

oscarriddle commented Mar 28, 2018

oscarriddle commented Mar 28, 2018

Noiredd commented Apr 19, 2018

oscarriddle commented Jul 3, 2018

oscarriddle commented Mar 20, 2018 •

edited