unpooled_height_ and unpooled_width_ missing -1 in calculation

[ZoroDerVonCodier]

At lines 101-102 of UnpoolingLayer::Reshape

unpooledheight = staticcast((height - 1) strideh + kernelh - 2 padh); unpooledwidth = staticcast((width - 1) stridew + kernelw - 2 padw);

shouldn't these instead read

unpooledheight = staticcast((height - 1) strideh + kernelh - 2 padh) - 1; unpooledwidth = staticcast((width - 1) stridew + kernelw - 2 padw) - 1;

so that they match (in reverse) the lines 90-93 of PoolingLayer::Reshape

pooledheight = static_cast(ceil(staticcast( height + 2 padh - kernelh) / strideh)) + 1; pooledwidth = static_cast(ceil(staticcast( width + 2 padw - kernelw) / stridew)) + 1;

I may have this wrong, but the unpooled calculations for height and width appear to be missing needed a -1 on each of unpooled height and width calculations.

[mariolew]

Hi, thanks for your comment, seems you're right.

[manuelschmidt]

Hmm actually I think it is correct without the additional -1 because it is already in the brackets (height_ -1). If you write the equation down on your paper you get exactly what is implemented without the additional -1. Best regards

[ZoroDerVonCodier]

I think you are partially correct but only when the ceil() is not used to actually 'round up' in the pooling layer.

For example, using the very simplified PROTOTXT, shows this error:

example { name: "Foo" input: "data" input_shape { dim: 1 dim: 3 dim: 56 dim: 56 } layer { name: "pool1" type: "Pooling" bottom: "data" top: "pool1" top: "pool1_mask" pooling_param { kernel_size: 4 stride: 4 pad: 0 pool: MAX } } layer { name: "pool2" type: "Pooling" bottom: "pool1" top: "pool2" top: "pool2_mask" pooling_param { kernel_size: 3 stride: 2 pad: 1 pool: MAX } } layer { name: "unpool2" type: "UnPooling" bottom: "pool2" bottom: "pool2_mask" top: "unpool2" pooling_param { kernel_size: 3 stride: 2 pad: 1 pool: MAX } } layer { name: "unpool1" type: "UnPooling" bottom: "unpool2" bottom: "pool1_mask" top: "unpool1" pooling_param { kernel_size: 4 stride: 4 pad: 0 pool: MAX } } }

I may be wrong on this but it appears that when the ceil() kicks in and actaully 'rounds up' in the pooling layer an extra '1' is added that must be accounted for in the unpooling.

When running the prototxt above the following values are observed:

pool1 layer: ht = 56, pad = 0, kernel = 4, stride = 4 pool1 poolht = ceil((56 + 2 * 0 - 4)/4) + 1 = ceil((56 + 0 - 4)/4) + 1 = ceil((52)/4) + 1 = ceil(13) + 1 = 13 + 1 <- ceiling not used for 52/4 = 13.0 = 14

pool2 layer: ht = 14, pad = 1, kernel = 3, stride = 2 pool2 poolht = ceil((14 + 2 * 1 - 3)/2) + 1 = ceil((14 + 2 - 3)/2) + 1 = ceil((16 - 3)/2) + 1 = ceil(13/2) + 1 = ceil(6.5) + 1 <- ceiling used to round 6.5 up to 7 = 7 + 1 = 8

unpool2 layer: ht = 8, pad = 1, kernel = 3, stride = 2 unpool2 upoolht = (int)((8 - 1) 2 + 3 - 2 1) = (int)((7) 2 + 3 - 2 1) = (int)(7 * 2 + 3 - 2) = (int)(14 + 1) = (int)(15) = 15 <- should be 14, does not account for extra 1 in ceil from pool2

This causes a memory overwrite in the mask which is sized with the 14x14 yet the forward operation uses the 15x15 size.

So I think the fix is to only subtract the extra 1 in the unpooling when the height (or width) are even numbers, something like this:

unpooledheight = staticcast((height - 1) strideh + kernelh - 2 padh) - ((height_ % 2 == 0) ? 1 : 0); unpooledwidth = staticcast((width - 1) stridew + kernelw - 2 padw) - ((width_ % 2 == 0) ? 1 : 0);

[manuelschmidt]

@ZoroDerVonCodier Thanks for the clarification! I think you got that one right - I totally missed the ceil() operation. Looks good to me now.

[ZoroDerVonCodier]

@manuelschmidt thank you for pushing me to dig a little deeper to get it right!

[ZoroDerVonCodier]

Requested change at lines 101-102 of UnpoolingLayer::Reshape

unpooledheight = staticcast((height - 1) strideh + kernelh - 2 padh) - ((height_ % 2 == 0) ? 1 : 0); unpooledwidth = staticcast((width - 1) stridew + kernelw - 2 padw) - ((width_ % 2 == 0) ? 1 : 0);

[mariolew]

@ZoroDerVonCodier @manuelschmidt Thanks for both of you. I'd be appreciated if you send a pull request to fix this issue.

[ZoroDerVonCodier]

pull request opened.

[ZoroDerVonCodier]

I think that there is one more fix that needs to be added to take into account lines 94-102 of the pooling_layer.cpp which are used to align the pooling to start 'strictly inside the image' when padding is used. This can cause the pooled height/width to be reduced by one to make the alignment.

Currently working on a fix. Suggestions welcome.

[mariolew]

@ZoroDerVonCodier, Thanks again for your attention. Do you have any idea to solve this?

[ZoroDerVonCodier]

working on it...

[ZoroDerVonCodier]

I created two pull requests: 1.) with the fix in the unpooling layer and 2.) additional tests in the test_unpooling_layer.cpp that now test the unpooling with padding.