SpatialAveragePadding padding and ceil kernels

[szagoruyko]

cuda part for torch/nn#365

[fmassa]

I think this condition is inverted. When you include the padding region, you should divide by pool_size.

[fmassa]

Actually, as is both conditions are the same. I think you should replace the ((hend - hstart) * (wend - wstart)) by kernel_h*kernel_w.

[fmassa]

I made some inline coments. Could you add some tests for the new cases ?

[hughperkins]

Tests are missing test for :ceil() mode?

[szagoruyko]

@hughperkins e040398

[hughperkins]

Ah :-) Right :-)

[hughperkins]

Hmmm, it looks like this compares with the cpu version? Does that mean there is a cpu implementation of ceil average pooling to compare with?

[fmassa]

@hughperkins yes, there is a CPU implementation, check torch/nn#365
I've written all the tests needed for both PRs to be merged. But there is still a problem with GPU implementation when count_include_pad is false, and I couldn't figure out why (in my tests, it seems that count_include_pad is always behaving as if it was true, even though we set it to false).

[fmassa]

(Note that I'm comparing it against my fork, with the corrections mentioned in this PR e040398 )

[hughperkins]

(ported to clnn, in branch, https://github.com/hughperkins/clnn/compare/master...avgpool?expand=1 ; pending merge of torch/nn#365 )

[fmassa]

I think that after https://github.com/szagoruyko/cunn/pull/2 is merged, this is fine to go.

[hughperkins]

Looks like this is ready to be merged right? :-)

[fmassa]

@hughperkins @szagoruyko was comparing this implementation against cudnn and it seems that the case in which setCountExcludePad is true there are some boundary differences. This case is not yet officially present in cudnn binding though, and I haven't had a look at it (still using CUDNN R2).

[hughperkins]

Can we at least merge in the nn version? Even though this isnt supported in cunn yet, but it is supported in clnn, but I cant merge it into clnn master until it is present in nn master.

[fmassa]

The thing is, both nn and cunn implemetations are equivalent, so if there is a case in which cunn does not behave as cudnn, then it's also going to be the case for nn.
I think what we all want is to have an equivalent implementation everywhere (and as cudnn is becoming the standard for CUDA modules, we need to give the same results as them).
I'll try to have a look at cudnn R3 version of setCountExcludePad, to see what they are actually computing, and I'll come back to you soon (hopefully today).

[hughperkins]

Ok... I think for my own usage I'm not using setCountExcludePad. Perhaps we could split this PR into two: one for ceilmode, and one for setCountExcludePad? An easy way to do that could be to just add an assert that doesnt allow anyone to use setCountExcludePad for now perhaps? (or to rename it to to unstable_unsupported_setCountExcludePad perhaps?)

[fmassa]

Hum, yeah, splitting the PR is a possibility... I'll have a look at it today when I have some time.

[szagoruyko]

this PR is almost ready to be merged, the only problem is the difference between it and COUNT_EXCLUDE_PADDING mode of cudnn, which we believe to be a bug in cudnn. will update as soon we find out more.

[hughperkins]

@szagoruyko Mmmkay. What if ... can we add something like assert(COUNT_EXCLUDE_PADDING == 0) for now? And then can make a later pull request to enable COUNT_EXCLUDE_PADDING? (I'm happy to submit the appropriate code change to disable COUNT_EXCLUDE_PADDING for now)

[fmassa]

I think this should be nInputCols >= kW - 2 * padW, and the same thing for H.
Currently, this condition doesn't allow the case of 1x1 input to be used with kW=kH=3 and padW=padH=1 (which I think should be valid).

[szagoruyko]

fixed Francisco's comments and squashed

[soumith]

Thanks Sergey.