-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Added param skip_propagate_down to LayerParameter #2095
Added param skip_propagate_down to LayerParameter #2095
Conversation
@jeffdonahue I see that somebody is tagging PR as "ready for review". Can you tag also this one? |
@jeffdonahue Can you pass here? |
@@ -192,7 +192,8 @@ class Net { | |||
/// @brief Append a new bottom blob to the net. | |||
int AppendBottom(const NetParameter& param, const int layer_id, | |||
const int bottom_id, set<string>* available_blobs, | |||
map<string, int>* blob_name_to_idx); | |||
map<string, int>* blob_name_to_idx, | |||
bool skip_propagate = false); |
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.
There's no reason to make skip_propagate
a new input -- just get it from layer_param
in AppendBottom
. (I don't think there should be any changes to Net::Init
if you do it this way, but I might have overlooked something.)
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 can change it if you want, but in this way this check needs to be done in the AppendBottom
function, that it is called Bottom.size()
times.
I know that this is not a computational relevant part, but don't you think that will be a little bit dirty?
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.
Leaving the check in Net::Init
is fine (though I'd also be fine with moving it to AppendBottom
). The rest of the logic is more natural to move to AppendBottom
(and its signature shouldn't change) since it's associated specifically with a new bottom blob, and Net::Init
is already longer than a single method should be.
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'll move the param retrieval into AppendBottom
and I'll leave the check into Net::Init
@jeffdonahue I apported the changes you suggested:
|
hi @jeffdonahue, any news? |
Added param skip_propagate_down to LayerParameter
// if propagate_down is true, the loss layer will try to | ||
// backpropagate on labels | ||
CHECK_EQ(need_back, true) | ||
<< "bottom_need_backward should be True"; |
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.
All the CHECK_EQ(.., true)
s in this test should be EXPECT_TRUE
(and CHECK_EQ(..., false)
-> EXPECT_FALSE
) -- the tests should fail but not die if the behavior being tested is incorrect.
@jeffdonahue We want to program our activity to reserve a little bit of time for this. Do you need other changes or is it mergeable? |
LGTM -- please squash to a single commit and I'll merge. Thanks @mtamburrano! |
1317671
to
c7c4c64
Compare
Done |
Added param skip_propagate_down to LayerParameter
Thanks @bhack and @mtamburrano! |
Another brick in the (merge) wall :) |
Added
skip_propagate_down
parameter as discussed with @jeffdonahue in #2054.The parameter can be specified either 0 or N times, where N is the number of bottom blobs.
if
skip_propagate_down[i] == True
then nothing will be backpropagated tobottom[i]
.If
force_backward
is specified, thenskip_propagate_down
has no effect.Tests are included in this PR in
test.net.cpp