Skip to content

Conversation

@ronghanghu
Copy link
Member

Implemented rectangular pooling, allowing pooling with different kernel_size, stride and pad along each axis.

Tests on rectangular pooling kernel regions have been added.

Add pad_h, pad_w, kernel_size_h, kernel_size_w, stride_h, stride_w to support pooling on rectangle regions.
Replace pad_, kernel_size_, stride_ with pad_h_, pad_w_, kernel_size_h_, kernel_size_w_, stride_h_, stride_w_ to support pooling on rectangle regions.
Replace pad_, kernel_size_, stride_ with pad_h_, pad_w_, kernel_size_h_, kernel_size_w_, stride_h_, stride_w_ to support pooling on rectangle regions.
Replace pad_, kernel_size_, stride_ with pad_h_, pad_w_, kernel_size_h_, kernel_size_w_, stride_h_, stride_w_ to support pooling on rectangle regions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that when kernel_size is specified, the other two are ignored. So why not both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For compatibility issue with previous caffe prototype file, we don't want to require users to update their network prototype. So, the user can either provide one parameter kernel_size or two parameters {kernel_size_w, kernel_size_h}. You may see #505 for detailed reasons.

@kloudkl
Copy link
Contributor

kloudkl commented Jul 4, 2014

Very useful generalization! Are you interested in further adding support for rectangular images?

@ronghanghu
Copy link
Member Author

Rectangular pooling kernel regions have been added to TestCPUForwardMax, TestGPUForwardMax, TestCPUForwardMaxTopMask, TestGPUForwardMaxTopMask in test_pooling_layer.cpp

@kloudkl
Copy link
Contributor

kloudkl commented Jul 5, 2014

Why are the first four commits repeated twice? The two commits with the message "fixing pooling SetUp() to allow default values for stride and pad" had better be squashed into one.

@ronghanghu
Copy link
Member Author

@shelhamer @jeffdonahue would you have a look the changes and see if this PR can be merged?

@jeffdonahue
Copy link
Contributor

Hey @ronghanghu, this looks great! Please run make lint to see style errors and fix these. Also, please add gradient check tests for the non-square case (you only have tests for the forward pass).

@ronghanghu
Copy link
Member Author

style errors are fixed and gradient checks are added for non-square pooling.

@jeffdonahue
Copy link
Contributor

Perfect, thanks Ronghang.

jeffdonahue added a commit that referenced this pull request Jul 7, 2014
@jeffdonahue jeffdonahue merged commit d82a1ea into BVLC:dev Jul 7, 2014
@ronghanghu ronghanghu deleted the rectangular_pooling branch July 7, 2014 23:53
@ronghanghu ronghanghu restored the rectangular_pooling branch July 7, 2014 23:53
@shelhamer shelhamer mentioned this pull request Aug 7, 2014
@ronghanghu ronghanghu deleted the rectangular_pooling branch August 18, 2014 21:29
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
RazvanRanca pushed a commit to RazvanRanca/caffe that referenced this pull request Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants