From a573dd4cc6f5a41ddbeec1be560d587f61029005 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 13:21:12 +0800 Subject: [PATCH 1/9] Use ostream << operator to get to_string * Make `PADDLE_ENFORCE_EQ` supports custom class, like DDim --- paddle/platform/enforce.h | 7 ++--- paddle/platform/enforce_test.cc | 40 +++++++++++++++++++++++++++- paddle/string/CMakeLists.txt | 1 + paddle/string/to_string.h | 40 ++++++++++++++++++++++++++++ paddle/string/to_string_test.cc | 46 +++++++++++++++++++++++++++++++++ 5 files changed, 130 insertions(+), 4 deletions(-) create mode 100644 paddle/string/to_string.h create mode 100644 paddle/string/to_string_test.cc diff --git a/paddle/platform/enforce.h b/paddle/platform/enforce.h index d2adb997de..337a059fb1 100644 --- a/paddle/platform/enforce.h +++ b/paddle/platform/enforce.h @@ -15,11 +15,12 @@ limitations under the License. */ #pragma once #include -#include #include #include #include #include +#include "paddle/string/printf.h" +#include "paddle/string/to_string.h" #ifndef PADDLE_ONLY_CPU @@ -194,8 +195,8 @@ inline void throw_on_error(T e) { #define __PADDLE_BINARY_COMPARE(__VAL0, __VAL1, __CMP, __INV_CMP, ...) \ PADDLE_ENFORCE(__VAL0 __CMP __VAL1, \ "enforce %s " #__CMP " %s failed, %s " #__INV_CMP " %s\n%s", \ - #__VAL0, #__VAL1, std::to_string(__VAL0), \ - std::to_string(__VAL1), \ + #__VAL0, #__VAL1, paddle::string::to_string(__VAL0), \ + paddle::string::to_string(__VAL1), \ paddle::string::Sprintf("" __VA_ARGS__)); } // namespace platform diff --git a/paddle/platform/enforce_test.cc b/paddle/platform/enforce_test.cc index 5408fce558..80bdee3d9d 100644 --- a/paddle/platform/enforce_test.cc +++ b/paddle/platform/enforce_test.cc @@ -9,6 +9,8 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ +#include +#include #include #include "gtest/gtest.h" @@ -83,7 +85,7 @@ TEST(ENFORCE_NE, FAIL) { } catch (paddle::platform::EnforceNotMet error) { caught_exception = true; EXPECT_TRUE(HasPrefix(StringPiece(error.what()), - "enforce 1.0 != 1UL failed, 1.000000 == 1")) + "enforce 1.0 != 1UL failed, 1 == 1")) << error.what() << " does not have expected prefix"; } EXPECT_TRUE(caught_exception); @@ -176,3 +178,39 @@ TEST(ENFORCE_NOT_NULL, FAIL) { } EXPECT_TRUE(caught_exception); } + +struct Dims { + size_t dims_[4]; + + bool operator==(const Dims& o) const { + for (size_t i = 0; i < 4; ++i) { + if (dims_[i] != o.dims_[i]) return false; + } + return true; + } +}; + +std::ostream& operator<<(std::ostream& os, const Dims& d) { + for (size_t i = 0; i < 4; ++i) { + if (i == 0) { + os << "["; + } + os << d.dims_[i]; + if (i == 4 - 1) { + os << "]"; + } else { + os << ", "; + } + } + return os; +} + +TEST(ENFORCE_USER_DEFINED_CLASS, EQ) { + Dims a{{1, 2, 3, 4}}, b{{1, 2, 3, 4}}; + PADDLE_ENFORCE_EQ(a, b); +} + +TEST(ENFORCE_USER_DEFINED_CLASS, NE) { + Dims a{{1, 2, 3, 4}}, b{{5, 6, 7, 8}}; + ASSERT_THROW(PADDLE_ENFORCE_EQ(a, b), paddle::platform::EnforceNotMet); +} \ No newline at end of file diff --git a/paddle/string/CMakeLists.txt b/paddle/string/CMakeLists.txt index 5becf62672..60667b7287 100644 --- a/paddle/string/CMakeLists.txt +++ b/paddle/string/CMakeLists.txt @@ -2,3 +2,4 @@ cc_library(stringpiece SRCS piece.cc) cc_test(stringpiece_test SRCS piece_test.cc DEPS stringpiece glog gflags) cc_test(stringprintf_test SRCS printf_test.cc DEPS glog gflags) +cc_test(to_string_test SRCS to_string_test.cc) diff --git a/paddle/string/to_string.h b/paddle/string/to_string.h new file mode 100644 index 0000000000..4f478b6a36 --- /dev/null +++ b/paddle/string/to_string.h @@ -0,0 +1,40 @@ +/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. */ + +#pragma once +#include +#include + +namespace paddle { +namespace string { +template +inline std::string to_string(T v) { + std::ostringstream sout; + sout << v; + return sout.str(); +} + +// Faster std::string/const char* type +template <> +inline std::string to_string(std::string v) { + return v; +} + +template <> +inline std::string to_string(const char* v) { + return std::string(v); +} + +} // namespace string +} // namespace paddle diff --git a/paddle/string/to_string_test.cc b/paddle/string/to_string_test.cc new file mode 100644 index 0000000000..0ef06eac24 --- /dev/null +++ b/paddle/string/to_string_test.cc @@ -0,0 +1,46 @@ +/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. */ + +#include "paddle/string/to_string.h" +#include + +constexpr char OUT_STR[] = "User Defined Output"; +class UserDefinedClass { +public: +}; + +std::ostream& operator<<(std::ostream& s, const UserDefinedClass& ins) { + s << OUT_STR; + return s; +} + +TEST(to_string, normal) { + using namespace paddle::string; + ASSERT_EQ(std::to_string(10), to_string(10)); + ASSERT_EQ("abc", to_string("abc")); + + auto std_to_string = std::to_string(1.2); + auto my_to_string = to_string(1.2); + + // std::to_string might fill zero after float value, like 1.2000 + for (size_t i = 0; i < my_to_string.size(); ++i) { + ASSERT_EQ(my_to_string[i], std_to_string[i]); + } +} + +TEST(to_string, user_defined) { + using namespace paddle::string; + UserDefinedClass instance; + ASSERT_EQ(OUT_STR, to_string(instance)); +} \ No newline at end of file From e67a1c928d6ee3c0588d6b31c510c3e41ef83b38 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 13:59:07 +0800 Subject: [PATCH 2/9] Make android compile pass --- paddle/string/to_string_test.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/paddle/string/to_string_test.cc b/paddle/string/to_string_test.cc index 0ef06eac24..57b4010626 100644 --- a/paddle/string/to_string_test.cc +++ b/paddle/string/to_string_test.cc @@ -25,6 +25,11 @@ std::ostream& operator<<(std::ostream& s, const UserDefinedClass& ins) { return s; } +// android macro comes from +// https://stackoverflow.com/questions/15328751/android-macro-suddenly-not-defined +#if !defined(ANDROID) && !defined(__ANDROID__) +// In android, std::to_string is not defined. +// https://stackoverflow.com/questions/22774009/android-ndk-stdto-string-support TEST(to_string, normal) { using namespace paddle::string; ASSERT_EQ(std::to_string(10), to_string(10)); @@ -38,6 +43,7 @@ TEST(to_string, normal) { ASSERT_EQ(my_to_string[i], std_to_string[i]); } } +#endif TEST(to_string, user_defined) { using namespace paddle::string; From d8a3291d87f20b6e4973bd9735e7a761752a10f1 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 14:14:47 +0800 Subject: [PATCH 3/9] Refine unit-test for to_string --- paddle/string/to_string_test.cc | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/paddle/string/to_string_test.cc b/paddle/string/to_string_test.cc index 57b4010626..4a075751ac 100644 --- a/paddle/string/to_string_test.cc +++ b/paddle/string/to_string_test.cc @@ -25,25 +25,12 @@ std::ostream& operator<<(std::ostream& s, const UserDefinedClass& ins) { return s; } -// android macro comes from -// https://stackoverflow.com/questions/15328751/android-macro-suddenly-not-defined -#if !defined(ANDROID) && !defined(__ANDROID__) -// In android, std::to_string is not defined. -// https://stackoverflow.com/questions/22774009/android-ndk-stdto-string-support TEST(to_string, normal) { using namespace paddle::string; - ASSERT_EQ(std::to_string(10), to_string(10)); + ASSERT_EQ("10", to_string(10)); ASSERT_EQ("abc", to_string("abc")); - - auto std_to_string = std::to_string(1.2); - auto my_to_string = to_string(1.2); - - // std::to_string might fill zero after float value, like 1.2000 - for (size_t i = 0; i < my_to_string.size(); ++i) { - ASSERT_EQ(my_to_string[i], std_to_string[i]); - } + ASSERT_EQ("1.2", to_string(1.2)); } -#endif TEST(to_string, user_defined) { using namespace paddle::string; From 2d35c7008117cc2ec7c1a079947fa4537d6d2f58 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 14:29:24 +0800 Subject: [PATCH 4/9] Fit google name style --- paddle/string/to_string_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/paddle/string/to_string_test.cc b/paddle/string/to_string_test.cc index 4a075751ac..5ff1b007f1 100644 --- a/paddle/string/to_string_test.cc +++ b/paddle/string/to_string_test.cc @@ -15,13 +15,13 @@ #include "paddle/string/to_string.h" #include -constexpr char OUT_STR[] = "User Defined Output"; +constexpr char kOutputString[] = "User Defined Output"; class UserDefinedClass { public: }; std::ostream& operator<<(std::ostream& s, const UserDefinedClass& ins) { - s << OUT_STR; + s << kOutputString; return s; } @@ -35,5 +35,5 @@ TEST(to_string, normal) { TEST(to_string, user_defined) { using namespace paddle::string; UserDefinedClass instance; - ASSERT_EQ(OUT_STR, to_string(instance)); + ASSERT_EQ(kOutputString, to_string(instance)); } \ No newline at end of file From b228b463fa6f1a4cf1f102dcea1eff61f16cc698 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 15:09:57 +0800 Subject: [PATCH 5/9] Make const variables in operator.h fit google style * No POD instance is forbidden in global scope. See https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables --- paddle/framework/backward.cc | 6 ++-- paddle/framework/backward_test.cc | 31 +++++++++--------- paddle/framework/grad_op_builder_test.cc | 41 +++++++++++------------- paddle/framework/operator.h | 8 ++--- paddle/operators/mean_op.cc | 2 +- paddle/operators/mean_op.h | 4 +-- 6 files changed, 44 insertions(+), 48 deletions(-) diff --git a/paddle/framework/backward.cc b/paddle/framework/backward.cc index 47983110fa..be6656792f 100644 --- a/paddle/framework/backward.cc +++ b/paddle/framework/backward.cc @@ -133,8 +133,8 @@ std::shared_ptr BackwardRecursive( std::shared_ptr grad_op = OpRegistry::CreateGradOp(forwardOp); for (std::string& grad_input : grad_op->inputs_) { if (no_grad_names.count(grad_input)) { - std::string prefix = - grad_input.substr(0, grad_input.size() - kGradVarSuffix.size()); + std::string prefix = grad_input.substr( + 0, grad_input.size() - sizeof(kGradVarSuffix) / sizeof(char)); grad_input = prefix + kZeroVarSuffix; // If part of input gradient of that operator is not calculated, fill @@ -167,7 +167,7 @@ std::shared_ptr Backward( std::unordered_set no_grad_names; no_grad_names.reserve(no_grad_vars.size()); - no_grad_names.insert(kEmptyVarName + kGradVarSuffix); + no_grad_names.insert(std::string(kEmptyVarName) + kGradVarSuffix); for (auto& name : no_grad_vars) { no_grad_names.insert(name + kGradVarSuffix); diff --git a/paddle/framework/backward_test.cc b/paddle/framework/backward_test.cc index 6d5835bd22..1677a3ed4c 100644 --- a/paddle/framework/backward_test.cc +++ b/paddle/framework/backward_test.cc @@ -171,10 +171,10 @@ TEST(Backward, simple_op_grad) { ASSERT_EQ(4UL, gop->inputs_.size()); ASSERT_EQ(f::kEmptyVarName, gop->inputs_[0]); ASSERT_EQ("rowwise_add_grad", gop->type_); - ASSERT_EQ("X" + f::kGradVarSuffix, gop->outputs_[0]); - ASSERT_EQ("b" + f::kGradVarSuffix, gop->outputs_[1]); + ASSERT_EQ(f::GradVarName("X"), gop->outputs_[0]); + ASSERT_EQ(f::GradVarName("b"), gop->outputs_[1]); - ASSERT_EQ("X" + f::kGradVarSuffix, gop->Output("X" + f::kGradVarSuffix)); + ASSERT_EQ(f::GradVarName("X"), gop->Output(f::GradVarName("X"))); } TEST(Backward, simple_op_not_need_grad) { @@ -182,7 +182,7 @@ TEST(Backward, simple_op_not_need_grad) { ASSERT_NE(fwd, nullptr); auto gop = f::Backward(*fwd, {"X"}); ASSERT_EQ(std::find(gop->outputs_.begin(), gop->outputs_.end(), - "X" + f::kGradVarSuffix), + f::GradVarName("X")), gop->outputs_.end()); auto no_input_gop = f::Backward(*fwd, {"X", "b"}); @@ -250,18 +250,18 @@ TEST(Backward, net_input_of_network_not_need_grad) { all_output.erase(f::kEmptyVarName); for (auto &out : {"W1", "b1", "hidden0", "W2", "b2"}) { - ASSERT_NE(all_output.find(out + f::kGradVarSuffix), all_output.end()); + ASSERT_NE(all_output.find(f::GradVarName(out)), all_output.end()); } // Not Generated X - ASSERT_EQ(all_output.find("X" + f::kGradVarSuffix), all_output.end()); + ASSERT_EQ(all_output.find(f::GradVarName("X")), all_output.end()); ASSERT_EQ(2UL, bwd_net->ops_.size()); ASSERT_TRUE(bwd_net->ops_[1]->IsNetOp()); auto first_fc_grad = static_cast(bwd_net->ops_[1].get()); ASSERT_EQ(3UL, first_fc_grad->ops_.size()); ASSERT_EQ(f::kEmptyVarName, - first_fc_grad->ops_[2]->Output("A" + f::kGradVarSuffix)); + first_fc_grad->ops_[2]->Output(f::GradVarName("A"))); } TEST(Backward, net_shared_weight) { @@ -313,15 +313,15 @@ TEST(Backward, op_part_of_output_are_not_need) { ASSERT_EQ(1UL, fill_zero.inputs_.size()); ASSERT_EQ("Z", fill_zero.inputs_[0]); ASSERT_EQ(1UL, fill_zero.outputs_.size()); - ASSERT_EQ("Z" + f::kZeroVarSuffix, fill_zero.outputs_[0]); + ASSERT_EQ(std::string("Z") + f::kZeroVarSuffix, fill_zero.outputs_[0]); auto &d_many_out = *net->ops_[1]; ASSERT_EQ("many_output_op_grad", d_many_out.type_); ASSERT_EQ(1UL + 2UL + 2UL, d_many_out.inputs_.size()); // I/O/OG - ASSERT_EQ("Z" + f::kZeroVarSuffix, d_many_out.Input("z" + f::kGradVarSuffix)); - ASSERT_EQ("Y" + f::kGradVarSuffix, d_many_out.Input("y" + f::kGradVarSuffix)); - ASSERT_EQ("X" + f::kGradVarSuffix, - d_many_out.Output("x" + f::kGradVarSuffix)); + ASSERT_EQ(std::string("Z") + f::kZeroVarSuffix, + d_many_out.Input(f::GradVarName("z"))); + ASSERT_EQ(f::GradVarName("Y"), d_many_out.Input(f::GradVarName("y"))); + ASSERT_EQ(f::GradVarName("X"), d_many_out.Output(f::GradVarName("x"))); } TEST(Backward, op_part_of_input_are_not_need) { @@ -331,10 +331,9 @@ TEST(Backward, op_part_of_input_are_not_need) { ASSERT_EQ(grad_mul.type_, "mul_grad"); ASSERT_EQ(grad_mul.inputs_.size(), 2UL + 1UL + 1UL); ASSERT_EQ(grad_mul.outputs_.size(), 2UL); - ASSERT_EQ(grad_mul.Output("A" + f::kGradVarSuffix), f::kEmptyVarName); - ASSERT_EQ(grad_mul.Output("B" + f::kGradVarSuffix), "b" + f::kGradVarSuffix); - ASSERT_EQ(grad_mul.Input("Out" + f::kGradVarSuffix), - "out" + f::kGradVarSuffix); + ASSERT_EQ(grad_mul.Output(f::GradVarName("A")), f::kEmptyVarName); + ASSERT_EQ(grad_mul.Output(f::GradVarName("B")), f::GradVarName("b")); + ASSERT_EQ(grad_mul.Input(f::GradVarName("Out")), f::GradVarName("out")); ASSERT_EQ(grad_mul.Input("A"), "a"); ASSERT_EQ(grad_mul.Input("B"), "b"); ASSERT_EQ(grad_mul.Input("Out"), "out"); diff --git a/paddle/framework/grad_op_builder_test.cc b/paddle/framework/grad_op_builder_test.cc index cf7143eba4..f1ebbae52f 100644 --- a/paddle/framework/grad_op_builder_test.cc +++ b/paddle/framework/grad_op_builder_test.cc @@ -83,21 +83,19 @@ TEST(GradOpBuilder, MutiInOut) { EXPECT_EQ(grad_test_op->Input("Out1"), "out1"); EXPECT_EQ(grad_test_op->Inputs("Out2_mult"), std::vector({"out2_1", "out2_2"})); - EXPECT_EQ(grad_test_op->Input("Out1" + f::kGradVarSuffix), - "out1" + f::kGradVarSuffix); - EXPECT_EQ(grad_test_op->Inputs("Out2_mult" + f::kGradVarSuffix), + EXPECT_EQ(grad_test_op->Input(f::GradVarName("Out1")), + f::GradVarName("out1")); + EXPECT_EQ(grad_test_op->Inputs(f::GradVarName("Out2_mult")), std::vector( - {"out2_1" + f::kGradVarSuffix, "out2_2" + f::kGradVarSuffix})); + {f::GradVarName("out2_1"), f::GradVarName("out2_2")})); ASSERT_EQ(grad_test_op->outputs_.size(), 5UL); - EXPECT_EQ(grad_test_op->Output("In1" + f::kGradVarSuffix), - "in1" + f::kGradVarSuffix); - EXPECT_EQ(grad_test_op->Outputs("In2_mult" + f::kGradVarSuffix), - std::vector({"in2_1" + f::kGradVarSuffix, - "in2_2" + f::kGradVarSuffix, - "in2_3" + f::kGradVarSuffix})); - EXPECT_EQ(grad_test_op->Output("In3" + f::kGradVarSuffix), - "in3" + f::kGradVarSuffix); + EXPECT_EQ(grad_test_op->Output(f::GradVarName("In1")), f::GradVarName("in1")); + EXPECT_EQ(grad_test_op->Outputs(f::GradVarName("In2_mult")), + std::vector({f::GradVarName("in2_1"), + f::GradVarName("in2_2"), + f::GradVarName("in2_3")})); + EXPECT_EQ(grad_test_op->Output(f::GradVarName("In3")), f::GradVarName("in3")); } TEST(GradOpBuilder, IOIgnoredInGradient) { @@ -119,19 +117,18 @@ TEST(GradOpBuilder, IOIgnoredInGradient) { EXPECT_EQ(grad_test_op->Inputs("Out1_mult"), std::vector({"out1_1", "out1_2"})); EXPECT_EQ(grad_test_op->Input("Out2"), f::kEmptyVarName); - EXPECT_EQ(grad_test_op->Inputs("Out1_mult" + f::kGradVarSuffix), + EXPECT_EQ(grad_test_op->Inputs(f::GradVarName("Out1_mult")), std::vector( - {"out1_1" + f::kGradVarSuffix, "out1_2" + f::kGradVarSuffix})); - EXPECT_EQ(grad_test_op->Input("Out2" + f::kGradVarSuffix), - "out2" + f::kGradVarSuffix); + {f::GradVarName("out1_1"), f::GradVarName("out1_2")})); + EXPECT_EQ(grad_test_op->Input(f::GradVarName("Out2")), + f::GradVarName("out2")); ASSERT_EQ(grad_test_op->outputs_.size(), 5UL); - EXPECT_EQ(grad_test_op->Output("In1" + f::kGradVarSuffix), - "in1" + f::kGradVarSuffix); - EXPECT_EQ(grad_test_op->Outputs("In2_mult" + f::kGradVarSuffix), + EXPECT_EQ(grad_test_op->Output(f::GradVarName("In1")), f::GradVarName("in1")); + EXPECT_EQ(grad_test_op->Outputs(f::GradVarName("In2_mult")), std::vector( - {"in2_1" + f::kGradVarSuffix, "in2_2" + f::kGradVarSuffix})); - EXPECT_EQ(grad_test_op->Outputs("In3_mult" + f::kGradVarSuffix), + {f::GradVarName("in2_1"), f::GradVarName("in2_2")})); + EXPECT_EQ(grad_test_op->Outputs(f::GradVarName("In3_mult")), std::vector( - {"in3_1" + f::kGradVarSuffix, "in3_2" + f::kGradVarSuffix})); + {f::GradVarName("in3_1"), f::GradVarName("in3_2")})); } diff --git a/paddle/framework/operator.h b/paddle/framework/operator.h index ceef9f028b..8949baf60e 100644 --- a/paddle/framework/operator.h +++ b/paddle/framework/operator.h @@ -33,19 +33,19 @@ namespace paddle { namespace framework { /// If a variable is a empty variable, that name will be used. -const std::string kEmptyVarName = "@EMPTY@"; +constexpr char kEmptyVarName[] = "@EMPTY@"; /// If a variable is a temporary variable, that name will be set in Python, /// but it will be convert to a unique name in scope after OpCreator. -const std::string kTempVarName = "@TEMP@"; +constexpr char kTempVarName[] = "@TEMP@"; /// If a variable's name has a certain suffix, it means that the /// variable is the gradient of another varibale. /// e.g. Variable "x@GRAD" is the gradient of varibale "x". -const std::string kGradVarSuffix = "@GRAD"; +constexpr char kGradVarSuffix[] = "@GRAD"; /// Variables with this suffix are supposed to be filled up with zeros. -const std::string kZeroVarSuffix = "@ZERO"; +constexpr char kZeroVarSuffix[] = "@ZERO"; inline std::string GradVarName(const std::string& var_name) { return var_name + kGradVarSuffix; diff --git a/paddle/operators/mean_op.cc b/paddle/operators/mean_op.cc index 997b0c514e..2ea049cb36 100644 --- a/paddle/operators/mean_op.cc +++ b/paddle/operators/mean_op.cc @@ -41,7 +41,7 @@ class MeanOpMaker : public framework::OpProtoAndCheckerMaker { class MeanGradOp : public framework::OperatorWithKernel { protected: void InferShape(const framework::InferShapeContext &ctx) const override { - ctx.Output("X" + framework::kGradVarSuffix) + ctx.Output(framework::GradVarName("X")) ->Resize(ctx.Input("X")->dims()); } }; diff --git a/paddle/operators/mean_op.h b/paddle/operators/mean_op.h index f3db0a29bb..e8595a14fa 100644 --- a/paddle/operators/mean_op.h +++ b/paddle/operators/mean_op.h @@ -48,10 +48,10 @@ template class MeanGradKernel : public framework::OpKernel { public: void Compute(const framework::ExecutionContext& context) const override { - auto OG = context.Input("Out" + framework::kGradVarSuffix); + auto OG = context.Input(framework::GradVarName("Out")); PADDLE_ENFORCE(framework::product(OG->dims()) == 1, "Mean Gradient should be scalar"); - auto IG = context.Output("X" + framework::kGradVarSuffix); + auto IG = context.Output(framework::GradVarName("X")); IG->mutable_data(context.GetPlace()); T ig_size = (T)framework::product(IG->dims()); From 6c7c4333f83b43de3c4cd6813cf6433bb563b56f Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 16:05:22 +0800 Subject: [PATCH 6/9] Fix TravisCI test --- paddle/framework/backward.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/paddle/framework/backward.cc b/paddle/framework/backward.cc index be6656792f..437a44a8aa 100644 --- a/paddle/framework/backward.cc +++ b/paddle/framework/backward.cc @@ -133,8 +133,9 @@ std::shared_ptr BackwardRecursive( std::shared_ptr grad_op = OpRegistry::CreateGradOp(forwardOp); for (std::string& grad_input : grad_op->inputs_) { if (no_grad_names.count(grad_input)) { + // +1 for \0 std::string prefix = grad_input.substr( - 0, grad_input.size() - sizeof(kGradVarSuffix) / sizeof(char)); + 0, grad_input.size() - sizeof(kGradVarSuffix) / sizeof(char) + 1); grad_input = prefix + kZeroVarSuffix; // If part of input gradient of that operator is not calculated, fill From c957445c72fd8f2c0354d8b430ef37f47ac3bc73 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 17:51:21 +0800 Subject: [PATCH 7/9] A better error message for gradient checker * Give which parameter, which element are wrong. And what max_diff is. --- paddle/framework/pybind.cc | 9 +++- .../v2/framework/tests/gradient_checker.py | 41 +++++++++++-------- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/paddle/framework/pybind.cc b/paddle/framework/pybind.cc index 915ffb1c00..9139a496ec 100644 --- a/paddle/framework/pybind.cc +++ b/paddle/framework/pybind.cc @@ -22,6 +22,7 @@ limitations under the License. */ #include "paddle/operators/net_op.h" #include "paddle/platform/enforce.h" #include "paddle/platform/place.h" +#include "paddle/string/to_string.h" #include "pybind11/numpy.h" #include "pybind11/pybind11.h" #include "pybind11/stl.h" @@ -205,9 +206,13 @@ All parameter, weight, gradient are variables in Paddle. }); // clang-format on - py::class_(m, "GPUPlace").def(py::init()); + py::class_(m, "GPUPlace") + .def(py::init()) + .def("__str__", string::to_string); - py::class_(m, "CPUPlace").def(py::init<>()); + py::class_(m, "CPUPlace") + .def(py::init<>()) + .def("__str__", string::to_string); py::class_> operator_base( m, "Operator"); diff --git a/python/paddle/v2/framework/tests/gradient_checker.py b/python/paddle/v2/framework/tests/gradient_checker.py index b73c4869d1..7c4eda5f30 100644 --- a/python/paddle/v2/framework/tests/gradient_checker.py +++ b/python/paddle/v2/framework/tests/gradient_checker.py @@ -92,15 +92,26 @@ def get_numeric_gradient(op, class GradientChecker(unittest.TestCase): - def __is_close(self, numeric_grads, scope, max_relative_error): + def __is_close(self, numeric_grads, scope, max_relative_error, msg_prefix): for name in numeric_grads: - op_grad = numpy.array( - scope.find_var(grad_var_name(name)).get_tensor()) - is_close = numpy.allclose( - numeric_grads[name], op_grad, rtol=max_relative_error, atol=100) - if not is_close: - return False - return True + b = numpy.array(scope.find_var(grad_var_name(name)).get_tensor()) + a = numeric_grads[name] + + abs_a = numpy.abs(a) + # if abs_a is nearly zero, then use abs error for a, not relative + # error. + abs_a[abs_a < 1e-3] = 1 + + diff_mat = numpy.abs(a - b) / abs_a + max_diff = numpy.max(diff_mat) + + def err_msg(): + offset = numpy.argmax(diff_mat > max_relative_error) + return "%s Variable %s max gradient diff %f over limit %f, the first " \ + "error element is %d" % ( + msg_prefix, name, max_diff, max_relative_error, offset) + + self.assertLessEqual(max_diff, max_relative_error, err_msg()) def check_grad(self, forward_op, @@ -145,7 +156,8 @@ class GradientChecker(unittest.TestCase): # get numeric gradient for check_name in inputs_to_check: numeric_grad[check_name] = \ - get_numeric_gradient(forward_op, input_vars, output_name, check_name) + get_numeric_gradient(forward_op, input_vars, output_name, + check_name) # get operator gradient according to different device for place in places: @@ -187,15 +199,8 @@ class GradientChecker(unittest.TestCase): backward_op.infer_shape(scope) backward_op.run(scope, ctx) - if isinstance(place, core.CPUPlace): - msg = "CPU kernel gradient is not close to numeric gradient" - else: - if isinstance(place, core.GPUPlace): - msg = "GPU kernel gradient is not close to numeric gradient" - else: - raise ValueError("unknown place " + type(place)) - self.assertTrue( - self.__is_close(numeric_grad, scope, max_relative_error), msg) + self.__is_close(numeric_grad, scope, max_relative_error, + "Gradient Check On %s" % str(place)) if __name__ == '__main__': From f0a85b08053440b9a49346f6d07cc106472c5c33 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 18:03:39 +0800 Subject: [PATCH 8/9] Rename __is_close -> assert_is_close() --- python/paddle/v2/framework/tests/gradient_checker.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/python/paddle/v2/framework/tests/gradient_checker.py b/python/paddle/v2/framework/tests/gradient_checker.py index 7c4eda5f30..aacc5e88fe 100644 --- a/python/paddle/v2/framework/tests/gradient_checker.py +++ b/python/paddle/v2/framework/tests/gradient_checker.py @@ -92,7 +92,8 @@ def get_numeric_gradient(op, class GradientChecker(unittest.TestCase): - def __is_close(self, numeric_grads, scope, max_relative_error, msg_prefix): + def assert_is_close(self, numeric_grads, scope, max_relative_error, + msg_prefix): for name in numeric_grads: b = numpy.array(scope.find_var(grad_var_name(name)).get_tensor()) a = numeric_grads[name] @@ -199,8 +200,8 @@ class GradientChecker(unittest.TestCase): backward_op.infer_shape(scope) backward_op.run(scope, ctx) - self.__is_close(numeric_grad, scope, max_relative_error, - "Gradient Check On %s" % str(place)) + self.assert_is_close(numeric_grad, scope, max_relative_error, + "Gradient Check On %s" % str(place)) if __name__ == '__main__': From 840d0c74025306985a814c1480851f69923b580a Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 9 Aug 2017 18:11:21 +0800 Subject: [PATCH 9/9] Remove unnecessary C++ operator test They are tested in Python --- paddle/operators/CMakeLists.txt | 3 --- paddle/operators/add_op_test.cc | 28 ---------------------------- paddle/operators/mean_op_test.cc | 25 ------------------------- paddle/operators/sgd_op_test.cc | 22 ---------------------- 4 files changed, 78 deletions(-) delete mode 100644 paddle/operators/add_op_test.cc delete mode 100644 paddle/operators/mean_op_test.cc delete mode 100644 paddle/operators/sgd_op_test.cc diff --git a/paddle/operators/CMakeLists.txt b/paddle/operators/CMakeLists.txt index 9e4026d1c6..af22229978 100644 --- a/paddle/operators/CMakeLists.txt +++ b/paddle/operators/CMakeLists.txt @@ -45,10 +45,8 @@ cc_library(net_op SRCS net_op.cc DEPS op_registry) cc_test(net_op_test SRCS net_op_test.cc DEPS net_op) op_library(add_op SRCS add_op.cc add_op.cu) -cc_test(add_op_test SRCS add_op_test.cc DEPS add_op) op_library(mean_op SRCS mean_op.cc mean_op.cu) -cc_test(mean_op_test SRCS mean_op_test.cc DEPS mean_op) op_library(mul_op SRCS mul_op.cc mul_op.cu) op_library(rowwise_add_op SRCS rowwise_add_op.cu rowwise_add_op.cc) @@ -59,7 +57,6 @@ op_library(cross_entropy_op SRCS cross_entropy_op.cc cross_entropy_op.cu) op_library(fill_zeros_like_op SRCS fill_zeros_like_op.cc fill_zeros_like_op.cu) op_library(sgd_op SRCS sgd_op.cc sgd_op.cu) -cc_test(sgd_op_test SRCS sgd_op_test.cc DEPS sgd_op) op_library(fc_op SRCS fc_op.cc diff --git a/paddle/operators/add_op_test.cc b/paddle/operators/add_op_test.cc deleted file mode 100644 index bf529defb2..0000000000 --- a/paddle/operators/add_op_test.cc +++ /dev/null @@ -1,28 +0,0 @@ -/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. */ - -#include -#define private public -#include "paddle/framework/op_registry.h" - -USE_OP(add_two); - -TEST(AddOp, GetOpProto) { - auto& protos = paddle::framework::OpRegistry::protos(); - auto it = protos.find("add_two"); - ASSERT_NE(it, protos.end()); - auto& op_creators = paddle::framework::OpRegistry::op_creators(); - auto it1 = op_creators.find("add_two_grad"); - ASSERT_NE(it1, op_creators.end()); -} diff --git a/paddle/operators/mean_op_test.cc b/paddle/operators/mean_op_test.cc deleted file mode 100644 index 375dcd50e1..0000000000 --- a/paddle/operators/mean_op_test.cc +++ /dev/null @@ -1,25 +0,0 @@ -/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. */ - -#include - -#include - -USE_OP(mean); - -TEST(MeanOp, GetOpProto) { - auto& protos = paddle::framework::OpRegistry::protos(); - auto it = protos.find("mean"); - ASSERT_NE(it, protos.end()); -} diff --git a/paddle/operators/sgd_op_test.cc b/paddle/operators/sgd_op_test.cc deleted file mode 100644 index 75137259f5..0000000000 --- a/paddle/operators/sgd_op_test.cc +++ /dev/null @@ -1,22 +0,0 @@ -/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. */ - -#include -#include -USE_OP(sgd); -TEST(SGDOp, GetOpProto) { - auto& protos = paddle::framework::OpRegistry::protos(); - auto it = protos.find("sgd"); - ASSERT_NE(it, protos.end()); -}