-
Notifications
You must be signed in to change notification settings - Fork 18.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Non-square Filters and Separated Stride and Padding #505
Conversation
stride_ = this->layer_param_.convolution_param().stride(); | ||
pad_ = this->layer_param_.convolution_param().pad(); | ||
ConvolutionParameter conv_param = this->layer_param_.convolution_param(); | ||
kernel_height_ = conv_param.kernel_size(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably CHECK_GT(conv_param.kernel_size_size(), 0)
first. Also CHECK_GT(kernel_height_ * kernel_width_, 0)
after setting those -- we should have had a similar check all along really (since kernel_size defaults to 0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Please see my comments regarding protobuf fields)
Maybe we can save old format by having:
if (conv_param.has_kernel_size()) {
kernel_height_ = conv_param.kernel_size();
kernel_height_ = conv_param.kernel_size();
} else {
CHECK_EQ(conv_param.rectangular_kernel_size_size(), 2)
<< "Must specify either kernel_size or rectangular_kernel_size (2 numbers).";
kernel_height_ = conv_param.rectangular_kernel_size(0);
kernel_height_ = conv_param.rectangular_kernel_size(1);
}
This may provide maximum backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I've added these checks.
Le mardi 17 juin 2014, Jeff Donahue notifications@github.com a écrit :
In src/caffe/layers/im2col_layer.cpp:
@@ -13,15 +13,28 @@ template
void Im2colLayer::SetUp(const vector<Blob>& bottom,
vector<Blob>* top) {
Layer::SetUp(bottom, top);
- kernel_size_ = this->layer_param_.convolution_param().kernel_size();
- stride_ = this->layer_param_.convolution_param().stride();
- pad_ = this->layer_param_.convolution_param().pad();
- ConvolutionParameter conv_param = this->layer_param_.convolution_param();
- kernel_height_ = conv_param.kernel_size(0);
should probably CHECK_GT(conv_param.kernel_size_size(), 0) first. Also CHECK_GT(kernel_height_
- kernel_width_, 0) after setting those -- we should have had a similar
check all along really (since kernel_size defaults to 0).—
Reply to this email directly or view it on GitHub
https://github.com/BVLC/caffe/pull/505/files#r13839327.
Have fun in Beijing Jeff and Evan :) |
Thanks Yangqing, and thanks for the advice about the fields. I'll introduce kernel_size_y, kernel_size_x, etc. I've added tests too, but Le jeudi 19 juin 2014, Yangqing Jia notifications@github.com a écrit :
|
Rebased to improve interface per #505 (comment). However, this isn't done. See todo list in the PR message. I'll give this another shot after I finish some other work. @buaaliyi you might try to debug the rectangular im2col test failure if you are still interested in this feature. |
Hi, I'd be happy to help if needed. |
Hey guys, I don't want to push, but are there any plans for this? Just to know if I should wait or not. |
This has stalled for now. If you debug the GPU mode im2col please Le jeudi 10 juillet 2014, rmanor notifications@github.com a écrit :
|
@shelhamer I spent a few hours on this and still haven't figured it out. Still trying... |
Hello everyone, I was looking to implement this functionality myself and came across this thread. Thanks @shelhamer for starting this. I found your implementation very helpful. As you mentioned it was failing few tests. I found some minor issues with your implementation which was causing the problem. They are listed below -
With the above changes I ran the tests and it passed all the tests! Sorry for writing a long response and thanks again for opening this issue. |
@ejaz-izy thank you for taking a look and figuring out the issues! I will incorporate your fixes and finish up this PR with a separable convolution test that convolves with rectangular filters. I'll be sure to credit you in the commit message once this is done and rebased for merge. I can confirm your fixes are correct -- I'm happy many-eyes came to the rescue here since I couldn't spot my mistake. |
while keeping everything working as-is.
Thanks to @ejaz-izy's debugging in BVLC#505 (comment)
Compute the G_x kernel of the Sobel operator as a full filter and as separable filters to check the rectangular filter output.
Non-square Filters and Separated Stride and Padding
Thanks @ejaz-izy!
On Sun, Jul 27, 2014 at 3:13 PM, ejaz-izy notifications@github.com wrote:
|
Thanks to @ejaz-izy's debugging in BVLC#505 (comment)
Non-square Filters and Separated Stride and Padding
Thanks to @ejaz-izy's debugging in BVLC#505 (comment)
Non-square Filters and Separated Stride and Padding
Thanks to @ejaz-izy's debugging in BVLC/caffe#505 (comment)
Accept pairs of height/Y and width/X values for kernel size, stride, and pad in lieu of a single, shared value.
Of course the square/equal case still works and the old defaults (stride = 1 and padding = 0) are kept.
Open to comments on implementation and style.