A bug in pooling_layer.cpp?

[freeniliang]

@csukuangfj @Noiredd

Issue summary

A bug in pooling_layer.cpp?

original code:

if (pad_h_ || pad_w_) {
// If we have padding, ensure that the last pooling starts strictly
// inside the image (instead of at the padding); otherwise clip the last.`
if ((pooled_height_ - 1) * stride_h_ >= height_ + pad_h_) {
--pooled_height_;
}
if ((pooled_width_ - 1) * stride_w_ >= width_ + pad_w_) {
--pooled_width_;
}
CHECK_LT((pooled_height_ - 1) * stride_h_, height_ + pad_h_);
CHECK_LT((pooled_width_ - 1) * stride_w_, width_ + pad_w_);
}
I think that “if (pad_h_ || pad_w_) ” should be removed！

For example：
Input feature map = hiwi= 66，kernel=1，stride=2，pad=0，round_mode=CEIL;
ho=ceil_div((6+2*0-1) ,2)+1=4
(ho-1)*stride=(4-1)*2=6=hi+pad=6+0
In this case, the 4th output point is out side of the input map, ho/wo must be decreased by 1!

So, “if (pad_h_ || pad_w_) ” should be removed！

[csukuangfj]

caffe/src/caffe/layers/pooling_layer.cpp

Lines 107 to 118 in 04ab089

	if (pad_h_ || pad_w_) {
	// If we have padding, ensure that the last pooling starts strictly
	// inside the image (instead of at the padding); otherwise clip the last.
	if ((pooled_height_ - 1) * stride_h_ >= height_ + pad_h_) {
	--pooled_height_;
	}
	if ((pooled_width_ - 1) * stride_w_ >= width_ + pad_w_) {
	--pooled_width_;
	}
	CHECK_LT((pooled_height_ - 1) * stride_h_, height_ + pad_h_);
	CHECK_LT((pooled_width_ - 1) * stride_w_, width_ + pad_w_);
	}

I think that “if (pad_h_ || pad_w_) ” should be removed！

You'll fail at line 116

caffe/src/caffe/layers/pooling_layer.cpp

Line 116 in 04ab089

CHECK_LT((pooled_height_ - 1) * stride_h_, height_ + pad_h_);

In this case, the 4th output point is out side of the input map, ho/wo must be decreased by 1!

Pleas read the code further:

caffe/src/caffe/layers/pooling_layer.cpp

Lines 168 to 175 in 04ab089

	int hstart = ph * stride_h_ - pad_h_;
	int wstart = pw * stride_w_ - pad_w_;
	int hend = min(hstart + kernel_h_, height_);
	int wend = min(wstart + kernel_w_, width_);
	hstart = max(hstart, 0);
	wstart = max(wstart, 0);
	const int pool_index = ph * pooled_width_ + pw;
	for (int h = hstart; h < hend; ++h) {

At line 175, h < hend will be false and the for loop is not executed.

[freeniliang]

caffe/src/caffe/layers/pooling_layer.cpp

Lines 107 to 118 in 04ab089

	if (pad_h_ || pad_w_) {
	// If we have padding, ensure that the last pooling starts strictly
	// inside the image (instead of at the padding); otherwise clip the last.
	if ((pooled_height_ - 1) * stride_h_ >= height_ + pad_h_) {
	--pooled_height_;
	}
	if ((pooled_width_ - 1) * stride_w_ >= width_ + pad_w_) {
	--pooled_width_;
	}
	CHECK_LT((pooled_height_ - 1) * stride_h_, height_ + pad_h_);
	CHECK_LT((pooled_width_ - 1) * stride_w_, width_ + pad_w_);
	}

I think that “if (pad_h_ || pad_w_) ” should be removed！

You'll fail at line 116

caffe/src/caffe/layers/pooling_layer.cpp

Line 116 in 04ab089

CHECK_LT((pooled_height_ - 1) * stride_h_, height_ + pad_h_);

In this case, the 4th output point is out side of the input map, ho/wo must be decreased by 1!

Pleas read the code further:

caffe/src/caffe/layers/pooling_layer.cpp

Lines 168 to 175 in 04ab089

	int hstart = ph * stride_h_ - pad_h_;
	int wstart = pw * stride_w_ - pad_w_;
	int hend = min(hstart + kernel_h_, height_);
	int wend = min(wstart + kernel_w_, width_);
	hstart = max(hstart, 0);
	wstart = max(wstart, 0);
	const int pool_index = ph * pooled_width_ + pw;
	for (int h = hstart; h < hend; ++h) {

At line 175, h < hend will be false and the for loop is not executed.

---

---

“if (pad_h_ || pad_w_) ” should be removed！You'll fail at line 116
-----I think that line 116 will not fail, because pooled_height_/pooled_height_ is already decreased by 1, so, (pooled_height_ - 1) * stride_h_ must be less than height_ + pad_h_ at line 116;

At line 175, h < hend will be false and the for loop is not executed.
----- Yes, this loop will not be executed, but the number of output has been identified in the following below:

caffe/src/caffe/layers/pooling_layer.cpp

Lines 107 to 118 in 04ab089

	if (pad_h_ || pad_w_) {
	// If we have padding, ensure that the last pooling starts strictly
	// inside the image (instead of at the padding); otherwise clip the last.
	if ((pooled_height_ - 1) * stride_h_ >= height_ + pad_h_) {
	--pooled_height_;
	}
	if ((pooled_width_ - 1) * stride_w_ >= width_ + pad_w_) {
	--pooled_width_;
	}
	CHECK_LT((pooled_height_ - 1) * stride_h_, height_ + pad_h_);
	CHECK_LT((pooled_width_ - 1) * stride_w_, width_ + pad_w_);
	}

In this case, you will find that the data at the boundary(the most right/bottom) are invalid(AVE:0.0; MAX:-FLT_MAX).
In order to drop the invalid data, a slice layer must be added behind the pool layer, but if “if (pad_h_ || pad_w_) ” is removed, the output of pool layer can be used by next layer without inserting slice layer!