From d3d894337b2e42d125a877a712b2f44643531a4a Mon Sep 17 00:00:00 2001 From: Sam Skalicky Date: Thu, 19 Jul 2018 17:41:13 +0000 Subject: [PATCH 1/4] fix for bug #10868: _backward_softsign activation is incorrect problem was that softsign was computed using outputs instead of inputs added inputs to list, and changed what gets passed into softsign calculation added softsign test to test_activation function code reviewed by anirudh --- src/operator/nn/activation-inl.h | 2 +- src/operator/nn/activation.cc | 11 +++++++++-- tests/python/unittest/test_operator.py | 4 ++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/operator/nn/activation-inl.h b/src/operator/nn/activation-inl.h index e6f8915ab2ff..f3eff102ccf6 100644 --- a/src/operator/nn/activation-inl.h +++ b/src/operator/nn/activation-inl.h @@ -174,7 +174,7 @@ void ActivationGradComputeImpl(const nnvm::NodeAttrs& attrs, const OpContext &ct break; case activation::kSoftSign: ActivationBackward( - ctx, inputs[0], inputs[1], req[0], outputs[0]); + ctx, inputs[0], inputs[2], req[0], outputs[0]); break; default: LOG(FATAL) << "unknown activation type"; diff --git a/src/operator/nn/activation.cc b/src/operator/nn/activation.cc index d723bbe62d76..4973faefc124 100644 --- a/src/operator/nn/activation.cc +++ b/src/operator/nn/activation.cc @@ -44,11 +44,18 @@ struct ActivationGrad { const std::vector& ograds) const { std::vector heads(ograds.begin(), ograds.end()); heads.emplace_back(nnvm::NodeEntry{n, activation::kOut, 0}); -#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) + const NodeAttrs& attrs = n->attrs; + if (dmlc::get(attrs.parsed).act_type == activation::kSoftSign) { + // for softsign need the inputs to compute the activation. + heads.push_back(n->inputs[activation::kData]); + } + +#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) // for ReLU, no need to pass input data. This enables inplace optimization during the // forward pass. - if (dmlc::get(attrs.parsed).act_type != activation::kReLU) { + if (dmlc::get(attrs.parsed).act_type != activation::kReLU && + dmlc::get(attrs.parsed).act_type != activation::kSoftSign) { heads.push_back(n->inputs[activation::kData]); } #endif diff --git a/tests/python/unittest/test_operator.py b/tests/python/unittest/test_operator.py index 814266ad9aa3..1f8fe5eacc2b 100644 --- a/tests/python/unittest/test_operator.py +++ b/tests/python/unittest/test_operator.py @@ -6838,6 +6838,10 @@ def test_activation(): lambda x: np.log(1. + np.exp(x)), lambda x: 1. - 1 / (1 + np.exp(x)), -3.0, 3.0], + 'softsign': [lambda x: mx.sym.Activation(x, act_type='softsign'), + lambda x: x / (1. + np.abs(x)), + lambda x: 1. / np.square(1. + np.abs(x)), + -3.0, 3.0], } # Loop over operators for name, op in unary_ops.items(): From 44967142dcabdbb3ec3bf5de76735e0ba0a91373 Mon Sep 17 00:00:00 2001 From: Sam Skalicky Date: Thu, 19 Jul 2018 17:41:13 +0000 Subject: [PATCH 2/4] fix for bug #10868: _backward_softsign activation is incorrect problem was that softsign was computed using outputs instead of inputs added inputs to list, and changed what gets passed into softsign calculation added softsign test to test_activation function code reviewed by anirudh **amended to change tab to spaces --- src/operator/nn/activation-inl.h | 2 +- src/operator/nn/activation.cc | 11 +++++++++-- tests/python/unittest/test_operator.py | 4 ++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/operator/nn/activation-inl.h b/src/operator/nn/activation-inl.h index e6f8915ab2ff..f3eff102ccf6 100644 --- a/src/operator/nn/activation-inl.h +++ b/src/operator/nn/activation-inl.h @@ -174,7 +174,7 @@ void ActivationGradComputeImpl(const nnvm::NodeAttrs& attrs, const OpContext &ct break; case activation::kSoftSign: ActivationBackward( - ctx, inputs[0], inputs[1], req[0], outputs[0]); + ctx, inputs[0], inputs[2], req[0], outputs[0]); break; default: LOG(FATAL) << "unknown activation type"; diff --git a/src/operator/nn/activation.cc b/src/operator/nn/activation.cc index d723bbe62d76..3c71e37c8152 100644 --- a/src/operator/nn/activation.cc +++ b/src/operator/nn/activation.cc @@ -44,11 +44,18 @@ struct ActivationGrad { const std::vector& ograds) const { std::vector heads(ograds.begin(), ograds.end()); heads.emplace_back(nnvm::NodeEntry{n, activation::kOut, 0}); -#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) + const NodeAttrs& attrs = n->attrs; + if (dmlc::get(attrs.parsed).act_type == activation::kSoftSign) { + // for softsign need the inputs to compute the activation. + heads.push_back(n->inputs[activation::kData]); + } + +#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) // for ReLU, no need to pass input data. This enables inplace optimization during the // forward pass. - if (dmlc::get(attrs.parsed).act_type != activation::kReLU) { + if (dmlc::get(attrs.parsed).act_type != activation::kReLU && + dmlc::get(attrs.parsed).act_type != activation::kSoftSign) { heads.push_back(n->inputs[activation::kData]); } #endif diff --git a/tests/python/unittest/test_operator.py b/tests/python/unittest/test_operator.py index 814266ad9aa3..1f8fe5eacc2b 100644 --- a/tests/python/unittest/test_operator.py +++ b/tests/python/unittest/test_operator.py @@ -6838,6 +6838,10 @@ def test_activation(): lambda x: np.log(1. + np.exp(x)), lambda x: 1. - 1 / (1 + np.exp(x)), -3.0, 3.0], + 'softsign': [lambda x: mx.sym.Activation(x, act_type='softsign'), + lambda x: x / (1. + np.abs(x)), + lambda x: 1. / np.square(1. + np.abs(x)), + -3.0, 3.0], } # Loop over operators for name, op in unary_ops.items(): From 3f5d0ed5710653c11c6a9061a4e77ed8bbcc057d Mon Sep 17 00:00:00 2001 From: Sam Skalicky Date: Thu, 19 Jul 2018 21:08:34 +0000 Subject: [PATCH 3/4] rerunning the CI build From f53657c2d714f18846f510e4d23c48169a22dd10 Mon Sep 17 00:00:00 2001 From: Sam Skalicky Date: Thu, 19 Jul 2018 21:08:34 +0000 Subject: [PATCH 4/4] fixed size checks for when USE_MKLDNN=ON --- src/operator/nn/activation-inl.h | 5 +++-- src/operator/nn/activation.cc | 17 ++++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/operator/nn/activation-inl.h b/src/operator/nn/activation-inl.h index f3eff102ccf6..2705177f951d 100644 --- a/src/operator/nn/activation-inl.h +++ b/src/operator/nn/activation-inl.h @@ -198,12 +198,13 @@ void ActivationGradCompute(const nnvm::NodeAttrs& attrs, const std::vector& inputs, const std::vector& req, const std::vector& outputs) { -#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) const ActivationParam& param = nnvm::get(attrs.parsed); +#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) bool relu = param.act_type == activation::kReLU; CHECK_EQ(inputs.size(), relu ? 2U : 3U); #else - CHECK_EQ(inputs.size(), 2U); + bool softsign = param.act_type == activation::kSoftSign; + CHECK_EQ(inputs.size(), softsign ? 3U : 2U); #endif CHECK_EQ(outputs.size(), 1U); CHECK_EQ(req.size(), 1U); diff --git a/src/operator/nn/activation.cc b/src/operator/nn/activation.cc index 3c71e37c8152..3bf41db25d19 100644 --- a/src/operator/nn/activation.cc +++ b/src/operator/nn/activation.cc @@ -125,8 +125,8 @@ inline static bool BackwardActStorageType(const nnvm::NodeAttrs& attrs, std::vector *in_attrs, std::vector *out_attrs) { bool ret = false; -#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) const ActivationParam& param = nnvm::get(attrs.parsed); +#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1) if (param.act_type != activation::kReLU) { CHECK_EQ(in_attrs->size(), 3U); ret = ElemwiseStorageType<3, 1, false, false, false>(attrs, dev_mask, @@ -140,10 +140,17 @@ inline static bool BackwardActStorageType(const nnvm::NodeAttrs& attrs, in_attrs, out_attrs); } #else - CHECK_EQ(in_attrs->size(), 2U); - ret = ElemwiseStorageType<2, 1, false, false, false>(attrs, dev_mask, - dispatch_mode, - in_attrs, out_attrs); + if (param.act_type == activation::kSoftSign) { + CHECK_EQ(in_attrs->size(), 3U); + ret = ElemwiseStorageType<3, 1, false, false, false>(attrs, dev_mask, + dispatch_mode, + in_attrs, out_attrs); + } else { + CHECK_EQ(in_attrs->size(), 2U); + ret = ElemwiseStorageType<2, 1, false, false, false>(attrs, dev_mask, + dispatch_mode, + in_attrs, out_attrs); + } #endif CHECK_EQ(out_attrs->size(), 1U); #if MXNET_USE_MKLDNN == 1