-
Notifications
You must be signed in to change notification settings - Fork 18.6k
Filter layer #1482
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
Filter layer #1482
Conversation
include/caffe/common_layers.hpp
Outdated
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.
Is this out-of-date?
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.
yes it is, totally missed it, thanks
|
I took a first pass... it looks like the core implementation is there, but it needs some fixes as noted. I'm confused about whether |
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.
The selector should be (N \times 1 \times 1 \times 1), isn't it?
I'm not sure if the selector should be the first bottom blob or the last one.
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.
yes it should be, I'll edit this.
I don't see differences between first and last position, just say me what you prefer ;)
|
thank you for your detailed review, I'll fix the code waiting for your further replies where needed |
src/caffe/layers/filter_layer.cpp
Outdated
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.
Try to use predefined methods for offset
int data_offset_top = top[b-1]->offset(n);
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.
ok
|
I don't know how the test passed given the problems with lint, and the error with |
src/caffe/test/test_filter_layer.cpp
Outdated
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.
It should work independently of the sizeof(Dtype) and also for GPU
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.
I'm not sure about lint errors because make lint doesn't show anything (everything seems ok). About the indices_to_forward error, I think it access random data when n > indice_to_forward.size() and then always enter in (n != offset) case, I'll fix this.
|
pushed various fixes discussed. |
src/caffe/layers/filter_layer.cpp
Outdated
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.
I think CHECK_EQ(bottom[0]->num(), bottom[i]->num()) is more clear
src/caffe/layers/filter_layer.cpp
Outdated
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.
It is weird that propagate_down and need_back_prop don't align.
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.
they don't align because need_back_prop size is equals to bottom.size() - 1 (or top.size()), instead propagate_down has size equals to bottom.size().
This happens because bottom_selector never needs backprop and doesn't have a corresponding top, so is useless to force the user to specify '0' on the first element in the prototxt params
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.
Once the selector is the last bottom they will align.
|
@mtamburrano Have you tried what happen if you don't use |
src/caffe/layers/filter_layer.cu
Outdated
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.
add spaces around '+' and try fit in one line
Happens that blobs that not needs backprop (e.g. labels) receive it.
I'm not sure what are you proposing... We need to fill with zeros the diff matrix on indices of filtered items and these indices are stored in |
|
moved selector blob to last bottom position, so now top and bottom blobs indices are aligned. |
|
@sguada @longjon @shelhamer We need to allocate time on this in our weekly working plan. Can you give us some feedbacks of the review plan or what kind of work is still needed? |
|
@bhack @mtamburrano the introduction of Could you try to use two different filter layers, one for |
|
@sguada, I don't think this solution works. |
|
I saw a very very low activity by core developers (BVLC members) also after cvpr 2014 deadline. |
|
@shelhamer Can you pass here? |
|
closed, new PR is #2054 |
Filter_layer developed as discussed in #1448, implemented following the suggestions by @sguada and @longjon (paragraph 2(i)).
This layer accepts 2+ bottom blobs, the first blob acts as a selector, so it should be a vector s={0,1,2, .. n} in [0,1], and the remaining blobs are blobs to be filtered. Each value of 1 in the selector vector means that the items at the corresponding indices int the bottom_blobs[1+] will be kept, on the contrary a values of 0 means the items with corresponding symbols will be filtered out.
So only non-filtered items will be forwarded and afterwards will receive backpropagation.
As param the filter_layer needs a need_back_prop vector. This is needed because some blob could contains labels, and therefore will not need backpropagation. The need_back_prop vector as the same size as the top vector, and accepts 1s and 0s depending on whether it should be backpropagated or not.
This PR needs #1483 and #1484