-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-1234] Fix shape inference problems in Activation backward #13409
Changes from all commits
6de9148
d91a85a
af3006a
c965e1d
7f10239
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,13 +38,27 @@ const kwargs_t basic_activation_args = { }; | |
* \brief Generic bidirectional sanity test | ||
*/ | ||
TEST(ACTIVATION_PERF, ExecuteBidirectional) { | ||
using namespace std; | ||
TShape shape({5, 5}); | ||
kwargs_t kwargs = basic_activation_args; | ||
kwargs.push_back({"act_type", "tanh"}); | ||
|
||
test::op::CoreOperatorRunner<float> runner; | ||
runner.RunBidirectional(false, { shape }, test::op::CoreOpExecutor<float>::ArgsWithOpName( | ||
kwargs, "Activation", "_backward_Activation"), 1); | ||
vector<string> activations = { | ||
"relu", | ||
"sigmoid", | ||
"tanh", | ||
"softrelu", | ||
"softsign" | ||
}; | ||
for (const string& activation : activations) { | ||
kwargs_t activation_args = {{"act_type", activation}}; | ||
test::op::CoreOperatorRunner<float> runner; | ||
runner.RunBidirectional(false, { shape }, test::op::CoreOpExecutor<float>::ArgsWithOpName( | ||
activation_args, "Activation", "_backward_Activation"), 1); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried this test case on my machine with both MKL-DNN and CUDNN enabled, without the last commit. All the 5 activations run into MXNet CPU implementation (ActivationCompute<cpu> and ActivationGradCompute<cpu>). If set isGPU=true, all the 5 activations run into GPU implementation (ActivationCompute<gpu> and ActivationGradCompute<gpu>). Seems no MKL-DNN implementation was triggered for both isGPU=false and isGPU=true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @larroy How to reproduce "What I observed is that we are calculating the backward in MKLDNN even if we have CUDNN." ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TaoLv you are right, this statement is not true in the master branch. I retract it. We can use this patch to reproduce the bug and see what runs and doesn't run: then run: build/tests/mxnet_unit_tests --gtest_filter="ACTIVATION_PERF.ExecuteBidirectional"
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied this patch to mxnet master branch and got below error:
|
||
for (const string& activation : activations) { | ||
kwargs_t activation_args = {{"act_type", activation}}; | ||
test::op::CoreOperatorRunner<float> runner; | ||
runner.RunBidirectional(true, { shape }, test::op::CoreOpExecutor<float>::ArgsWithOpName( | ||
activation_args, "Activation", "_backward_Activation"), 1); | ||
} | ||
} | ||
|
||
/*! | ||
|
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.
Can we revert this change? The common practice is that if we change the elements in the vector, we will use pointer instead of const reference.
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.
Right. We are not changing the vector in this function if you look closely. So your comment doesn't apply.