From 02480316016bd04c10676d8cb859c473f07a819f Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 12 Jan 2017 17:18:39 +0800 Subject: [PATCH 01/13] Add Status --- paddle/utils/Status.h | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 paddle/utils/Status.h diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h new file mode 100644 index 00000000000000..398ae182ab053e --- /dev/null +++ b/paddle/utils/Status.h @@ -0,0 +1,39 @@ +/* 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 + +namespace paddle { + +class Status final : public std::exception { +public: + Status() noexcept {} + + Status(const std::string& msg) : errMsg_(new std::string(msg)) {} + + virtual const char* what() const noexcept override { + if (errMsg_) { + return errMsg_->data(); + } else { + return nullptr; + } + } + + inline bool isOK() const noexcept { return errMsg_ == nullptr; } + +private: + std::unique_ptr errMsg_; +}; + +} // namespace paddle From 6c20e08b042e1351a4ea8c97a74129d211b1d636 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 12 Jan 2017 17:55:36 +0800 Subject: [PATCH 02/13] Try using status to handle Paddle Error --- .../activations/ActivationFunction.cpp | 126 ++++++++++++++---- .../gserver/activations/ActivationFunction.h | 5 +- paddle/gserver/layers/Layer.cpp | 7 +- paddle/utils/Status.h | 36 ++++- paddle/utils/tests/CMakeLists.txt | 1 + paddle/utils/tests/test_Status.cpp | 29 ++++ 6 files changed, 169 insertions(+), 35 deletions(-) create mode 100644 paddle/utils/tests/test_Status.cpp diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index f8c4bcac2f8eb4..8a938cf7e9d730 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -69,8 +69,14 @@ static ClassRegistrar gActivationRegistrar; class IdentityActivation : public ActivationFunction { public: static const std::string name; - void forward(Argument& act) { (void)act; } - void backward(Argument& act) { (void)act; } + Status forward(Argument& act) { + (void)act; + return Status(); + } + Status backward(Argument& act) { + (void)act; + return Status(); + } const std::string& getName() const { return name; } }; const std::string IdentityActivation::name = ""; @@ -86,8 +92,14 @@ static InitFunction __reg_activation__identity([] { * \f] */ BEGIN_DEFINE_ACTIVATION(sigmoid) -void forward(Argument& act) { act.value->sigmoid(*act.value); } -void backward(Argument& act) { act.grad->sigmoidDerivative(*act.value); } +Status forward(Argument& act) { + act.value->sigmoid(*act.value); + return Status(); +} +Status backward(Argument& act) { + act.grad->sigmoidDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(sigmoid) /** @@ -103,9 +115,12 @@ MatrixPtr sftMaxDot_; MatrixPtr one_; public: -void forward(Argument& act) { act.value->softmax(*act.value); } +Status forward(Argument& act) { + act.value->softmax(*act.value); + return Status(); +} -void backward(Argument& act) { +Status backward(Argument& act) { MatrixPtr outputV = act.value; MatrixPtr outputG = act.grad; @@ -137,6 +152,7 @@ void backward(Argument& act) { act.grad->softmaxDerivative(*act.value, *sftMaxSum_); } + return Status(); } END_DEFINE_ACTIVATION(softmax) @@ -151,8 +167,11 @@ ACTIVATION_CLASS_NAME(softmax) softmax_; Argument argument_; public: -void forward(Argument& act) { - CHECK_EQ(act.value->getWidth(), 1UL); +Status forward(Argument& act) { + if (act.value->getWidth() != 1UL) { + return Status( + "Input width for each timestep of sequence softmax should be 1"); + } if (!argument_.value) { argument_.value = Matrix::create(nullptr, @@ -169,10 +188,14 @@ void forward(Argument& act) { auto starts = act.sequenceStartPositions->getVector(useGpu(act.deviceId)); act.value->sequenceSoftmax(*act.value, *starts); + return Status(); } -void backward(Argument& act) { - CHECK_EQ(act.grad->getWidth(), 1UL); +Status backward(Argument& act) { + if (act.value->getWidth() != 1UL) { + return Status( + "Input width for each timestep of sequence softmax should be 1"); + } size_t numSequences = act.getNumSequences(); const int* starts = act.sequenceStartPositions->getData(false); @@ -186,6 +209,7 @@ void backward(Argument& act) { softmax_.backward(argument_); } + return Status(); } END_DEFINE_ACTIVATION(sequence_softmax) @@ -200,9 +224,15 @@ END_DEFINE_ACTIVATION(sequence_softmax) * 0 otherwise. */ BEGIN_DEFINE_ACTIVATION(relu) -void forward(Argument& act) { act.value->relu(*act.value); } +Status forward(Argument& act) { + act.value->relu(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->reluDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->reluDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(relu) /** @@ -219,9 +249,15 @@ END_DEFINE_ACTIVATION(relu) * TODO(yuyang18): Remove magic number 24 or make it configuable. */ BEGIN_DEFINE_ACTIVATION(brelu) -void forward(Argument& act) { act.value->brelu(*act.value); } +Status forward(Argument& act) { + act.value->brelu(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->breluDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->breluDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(brelu) /** @@ -231,9 +267,15 @@ END_DEFINE_ACTIVATION(brelu) * \f] */ BEGIN_DEFINE_ACTIVATION(tanh) -void forward(Argument& act) { act.value->tanh(*act.value); } +Status forward(Argument& act) { + act.value->tanh(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->tanhDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->tanhDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(tanh) /** @@ -248,10 +290,14 @@ real a, b; public: ACTIVATION_CLASS_NAME(stanh)() : a(1.7159), b(2. / 3.) {} -void forward(Argument& act) { act.value->scaledTanh(*act.value, a, b); } +Status forward(Argument& act) { + act.value->scaledTanh(*act.value, a, b); + return Status(); +} -void backward(Argument& act) { +Status backward(Argument& act) { act.grad->scaledTanhDerivative(*act.value, a, b); + return Status(); } END_DEFINE_ACTIVATION(stanh) @@ -262,9 +308,15 @@ END_DEFINE_ACTIVATION(stanh) * \f] */ BEGIN_DEFINE_ACTIVATION(softrelu) -void forward(Argument& act) { act.value->softrelu(*act.value); } +Status forward(Argument& act) { + act.value->softrelu(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->softreluDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->softreluDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(softrelu) /** @@ -280,7 +332,7 @@ END_DEFINE_ACTIVATION(softrelu) * 0 if z=0 */ BEGIN_DEFINE_ACTIVATION(abs) -void forward(Argument& act) { +Status forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -290,9 +342,13 @@ void forward(Argument& act) { act.in->copyFrom(*act.value); act.value->abs2(*act.value); + return Status(); } -void backward(Argument& act) { act.grad->absDerivative(*act.in); } +Status backward(Argument& act) { + act.grad->absDerivative(*act.in); + return Status(); +} END_DEFINE_ACTIVATION(abs) /** @@ -302,7 +358,7 @@ END_DEFINE_ACTIVATION(abs) * \f] */ BEGIN_DEFINE_ACTIVATION(square) -void forward(Argument& act) { +Status forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -312,9 +368,13 @@ void forward(Argument& act) { act.in->copyFrom(*act.value); act.value->square2(*act.value); + return Status(); } -void backward(Argument& act) { act.grad->squareDerivative(*act.in); } +Status backward(Argument& act) { + act.grad->squareDerivative(*act.in); + return Status(); +} END_DEFINE_ACTIVATION(square) /** @@ -324,9 +384,15 @@ END_DEFINE_ACTIVATION(square) * \f] */ BEGIN_DEFINE_ACTIVATION(exponential) -void forward(Argument& act) { act.value->exp2(*act.value); } +Status forward(Argument& act) { + act.value->exp2(*act.value); + return Status(); +} -void backward(Argument& act) { act.grad->expDerivative(*act.value); } +Status backward(Argument& act) { + act.grad->expDerivative(*act.value); + return Status(); +} END_DEFINE_ACTIVATION(exponential) /** @@ -336,7 +402,7 @@ END_DEFINE_ACTIVATION(exponential) * \f] */ BEGIN_DEFINE_ACTIVATION(log) -void forward(Argument& act) { +Status forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -346,9 +412,13 @@ void forward(Argument& act) { act.in->copyFrom(*act.value); act.value->log2(*act.value); + return Status(); } -void backward(Argument& act) { act.grad->dotDiv(*act.grad, *act.in); } +Status backward(Argument& act) { + act.grad->dotDiv(*act.grad, *act.in); + return Status(); +} END_DEFINE_ACTIVATION(log) ActivationFunction* ActivationFunction::create(const std::string& type) { diff --git a/paddle/gserver/activations/ActivationFunction.h b/paddle/gserver/activations/ActivationFunction.h index 601e3b6c0cd401..ad395ac28da7d9 100644 --- a/paddle/gserver/activations/ActivationFunction.h +++ b/paddle/gserver/activations/ActivationFunction.h @@ -15,6 +15,7 @@ limitations under the License. */ #pragma once #include #include +#include "paddle/utils/Status.h" namespace paddle { @@ -48,7 +49,7 @@ class ActivationFunction { * * Usually, act is Layer::output_ */ - virtual void forward(Argument& act) = 0; + virtual Status forward(Argument& act) = 0; /** * @brief Backward propagaion @@ -57,7 +58,7 @@ class ActivationFunction { * - Before calling backward(), act.grad = dE / dy, where E is the error/cost * - After backward() returns, act.grad = dE / dx = (dE/dy) * (dy/dx) */ - virtual void backward(Argument& act) = 0; + virtual Status backward(Argument& act) = 0; virtual const std::string& getName() const = 0; }; diff --git a/paddle/gserver/layers/Layer.cpp b/paddle/gserver/layers/Layer.cpp index c47943f81c0158..06c936c3aec670 100644 --- a/paddle/gserver/layers/Layer.cpp +++ b/paddle/gserver/layers/Layer.cpp @@ -16,6 +16,7 @@ limitations under the License. */ #include "paddle/math/SparseMatrix.h" #include "paddle/utils/Logging.h" +#include "paddle/utils/Status.h" #include "AddtoLayer.h" #include "CRFLayer.h" @@ -334,7 +335,8 @@ void Layer::showOutputStats() { void Layer::forwardActivation() { /* activation */ - activation_->forward(output_); + auto status = activation_->forward(output_); + CHECK(status.isOK()) << status.what(); /* dropout */ if (config_.drop_rate() > 0) { @@ -372,7 +374,8 @@ void Layer::backwardActivation() { oGrad->dotMul(*oGrad, *dropOutMask_); } - activation_->backward(output_); + auto status = activation_->backward(output_); + CHECK(status.isOK()) << status.what(); } void Layer::forwardDropOut() { diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index 398ae182ab053e..3456d7b686f0d9 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -11,18 +11,44 @@ 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 { +/** + * Status is Paddle error code. It only contain a std::string as error message. + * Although Status inherits the std::exception, but do not throw it except you + * know what you are doing. + */ class Status final : public std::exception { public: + /** + * Default Status. OK + */ Status() noexcept {} - Status(const std::string& msg) : errMsg_(new std::string(msg)) {} + /** + * @brief Create Status with error message + * @param msg + */ + explicit Status(const std::string& msg) : errMsg_(new std::string(msg)) {} + + /** + * @brief set a error message for status. + * @param msg + */ + inline void set(const std::string& msg) noexcept { + errMsg_.reset(new std::string(msg)); + } - virtual const char* what() const noexcept override { + /** + * @brief what will return the error message. If status is OK, return nullptr. + */ + const char* what() const noexcept override { if (errMsg_) { return errMsg_->data(); } else { @@ -30,10 +56,14 @@ class Status final : public std::exception { } } + /** + * @brief isOK + * @return true if OK. + */ inline bool isOK() const noexcept { return errMsg_ == nullptr; } private: - std::unique_ptr errMsg_; + std::shared_ptr errMsg_; }; } // namespace paddle diff --git a/paddle/utils/tests/CMakeLists.txt b/paddle/utils/tests/CMakeLists.txt index 26fafbd1ab3f29..a1cc32668d5100 100644 --- a/paddle/utils/tests/CMakeLists.txt +++ b/paddle/utils/tests/CMakeLists.txt @@ -4,6 +4,7 @@ add_simple_unittest(test_CustomStackTrace) add_simple_unittest(test_ThreadBarrier) add_simple_unittest(test_SpinLock) add_simple_unittest(test_SIMDFlags) +add_simple_unittest(test_Status) add_executable( test_CustomStackTracePrint diff --git a/paddle/utils/tests/test_Status.cpp b/paddle/utils/tests/test_Status.cpp new file mode 100644 index 00000000000000..e2c2ae537d8b62 --- /dev/null +++ b/paddle/utils/tests/test_Status.cpp @@ -0,0 +1,29 @@ +/* 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/utils/Status.h" + +#include + +TEST(Status, testAll) { + paddle::Status status; + ASSERT_TRUE(status.isOK()); + status.set("I'm the error"); + ASSERT_FALSE(status.isOK()); + ASSERT_STREQ("I'm the error", status.what()); + + paddle::Status status2("error2"); + ASSERT_FALSE(status2.isOK()); + ASSERT_STREQ("error2", status2.what()); +} From 741637eba41f66b51bd1764900e75cc7d5bd9ce6 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Mon, 16 Jan 2017 16:14:29 +0800 Subject: [PATCH 03/13] Add printf method to Status. --- paddle/utils/Status.h | 23 +++++++++++++++++++++++ paddle/utils/tests/test_Status.cpp | 5 +++++ 2 files changed, 28 insertions(+) diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index 3456d7b686f0d9..db1edfb7c735fc 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -14,6 +14,7 @@ limitations under the License. */ #pragma once +#include #include #include @@ -45,6 +46,28 @@ class Status final : public std::exception { errMsg_.reset(new std::string(msg)); } + /** + * @brief set a error message for status. Use C style printf + * @param fmt + */ + template + inline void setByPrintf(const char* fmt, ARGS... args) noexcept { + constexpr size_t bufferSize = 4096; + char buffer[bufferSize]; + snprintf(buffer, bufferSize, fmt, args...); + errMsg_.reset(new std::string(buffer)); + } + + /** + * create a error status by C style printf. + */ + template + inline static Status printf(const char* fmt, ARGS... args) noexcept { + Status s; + s.setByPrintf(fmt, args...); + return s; + } + /** * @brief what will return the error message. If status is OK, return nullptr. */ diff --git a/paddle/utils/tests/test_Status.cpp b/paddle/utils/tests/test_Status.cpp index e2c2ae537d8b62..04cef095792c73 100644 --- a/paddle/utils/tests/test_Status.cpp +++ b/paddle/utils/tests/test_Status.cpp @@ -26,4 +26,9 @@ TEST(Status, testAll) { paddle::Status status2("error2"); ASSERT_FALSE(status2.isOK()); ASSERT_STREQ("error2", status2.what()); + + int i = 3; + auto status3 = paddle::Status::printf("error%d", i); + ASSERT_FALSE(status3.isOK()); + ASSERT_STREQ("error3", status3.what()); } From 8aefc30499e09729b5755fe8edfd32ba72a9baed Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Tue, 17 Jan 2017 10:59:27 +0800 Subject: [PATCH 04/13] Fix compile error. --- paddle/utils/Status.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index db1edfb7c735fc..52f312378eedaa 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -52,9 +52,9 @@ class Status final : public std::exception { */ template inline void setByPrintf(const char* fmt, ARGS... args) noexcept { - constexpr size_t bufferSize = 4096; - char buffer[bufferSize]; - snprintf(buffer, bufferSize, fmt, args...); + constexpr size_t kBufferSize = 4096; + char buffer[kBufferSize]; + snprintf(buffer, kBufferSize, fmt, args...); errMsg_.reset(new std::string(buffer)); } From 3d01c60e25c1b5874e6854ac565646d6ad9432d7 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Tue, 17 Jan 2017 15:44:09 +0800 Subject: [PATCH 05/13] Stash --- paddle/utils/Compiler.h | 0 paddle/utils/Status.h | 9 ++++++++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 paddle/utils/Compiler.h diff --git a/paddle/utils/Compiler.h b/paddle/utils/Compiler.h new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index 52f312378eedaa..cb66e4b225e92c 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -24,6 +24,13 @@ namespace paddle { * Status is Paddle error code. It only contain a std::string as error message. * Although Status inherits the std::exception, but do not throw it except you * know what you are doing. + * + * + * There are two styles to return status in Paddle. + * + * 1. Return Status + * + * */ class Status final : public std::exception { public: @@ -52,7 +59,7 @@ class Status final : public std::exception { */ template inline void setByPrintf(const char* fmt, ARGS... args) noexcept { - constexpr size_t kBufferSize = 4096; + constexpr size_t kBufferSize = 1024; // 1KB buffer char buffer[kBufferSize]; snprintf(buffer, kBufferSize, fmt, args...); errMsg_.reset(new std::string(buffer)); From 9bc12034002d0c0ca5f30bd1f11b30188978e327 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 12:45:00 +0800 Subject: [PATCH 06/13] Add more comments, also add __must_check. --- .../activations/ActivationFunction.cpp | 55 +++++++++--------- .../gserver/activations/ActivationFunction.h | 4 +- paddle/gserver/layers/Layer.cpp | 4 +- paddle/gserver/layers/MDLstmLayer.cpp | 25 +++++---- paddle/gserver/layers/NCELayer.cpp | 6 +- paddle/gserver/layers/RecurrentLayer.cpp | 21 +++---- .../layers/SelectiveFullyConnectedLayer.cpp | 3 +- paddle/gserver/tests/test_WarpCTCLayer.cpp | 4 +- paddle/utils/Compiler.h | 25 +++++++++ paddle/utils/Status.h | 56 +++++++++++++++++++ 10 files changed, 147 insertions(+), 56 deletions(-) diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index 8a938cf7e9d730..666c2e01c8bdaf 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -69,11 +69,11 @@ static ClassRegistrar gActivationRegistrar; class IdentityActivation : public ActivationFunction { public: static const std::string name; - Status forward(Argument& act) { + Status __must_check forward(Argument& act) { (void)act; return Status(); } - Status backward(Argument& act) { + Status __must_check backward(Argument& act) { (void)act; return Status(); } @@ -92,11 +92,11 @@ static InitFunction __reg_activation__identity([] { * \f] */ BEGIN_DEFINE_ACTIVATION(sigmoid) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->sigmoid(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->sigmoidDerivative(*act.value); return Status(); } @@ -115,12 +115,12 @@ MatrixPtr sftMaxDot_; MatrixPtr one_; public: -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->softmax(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { MatrixPtr outputV = act.value; MatrixPtr outputG = act.grad; @@ -167,7 +167,7 @@ ACTIVATION_CLASS_NAME(softmax) softmax_; Argument argument_; public: -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { if (act.value->getWidth() != 1UL) { return Status( "Input width for each timestep of sequence softmax should be 1"); @@ -191,7 +191,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { if (act.value->getWidth() != 1UL) { return Status( "Input width for each timestep of sequence softmax should be 1"); @@ -207,7 +207,8 @@ Status backward(Argument& act) { argument_.value->setData(act.value->getData() + offset, 1UL, size); argument_.grad->setData(act.grad->getData() + offset, 1UL, size); - softmax_.backward(argument_); + Status status = softmax_.backward(argument_); + if (!status.isOK()) return status; } return Status(); } @@ -224,12 +225,12 @@ END_DEFINE_ACTIVATION(sequence_softmax) * 0 otherwise. */ BEGIN_DEFINE_ACTIVATION(relu) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->relu(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->reluDerivative(*act.value); return Status(); } @@ -249,12 +250,12 @@ END_DEFINE_ACTIVATION(relu) * TODO(yuyang18): Remove magic number 24 or make it configuable. */ BEGIN_DEFINE_ACTIVATION(brelu) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->brelu(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->breluDerivative(*act.value); return Status(); } @@ -267,12 +268,12 @@ END_DEFINE_ACTIVATION(brelu) * \f] */ BEGIN_DEFINE_ACTIVATION(tanh) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->tanh(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->tanhDerivative(*act.value); return Status(); } @@ -290,12 +291,12 @@ real a, b; public: ACTIVATION_CLASS_NAME(stanh)() : a(1.7159), b(2. / 3.) {} -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->scaledTanh(*act.value, a, b); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->scaledTanhDerivative(*act.value, a, b); return Status(); } @@ -308,12 +309,12 @@ END_DEFINE_ACTIVATION(stanh) * \f] */ BEGIN_DEFINE_ACTIVATION(softrelu) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->softrelu(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->softreluDerivative(*act.value); return Status(); } @@ -332,7 +333,7 @@ END_DEFINE_ACTIVATION(softrelu) * 0 if z=0 */ BEGIN_DEFINE_ACTIVATION(abs) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -345,7 +346,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->absDerivative(*act.in); return Status(); } @@ -358,7 +359,7 @@ END_DEFINE_ACTIVATION(abs) * \f] */ BEGIN_DEFINE_ACTIVATION(square) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -371,7 +372,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->squareDerivative(*act.in); return Status(); } @@ -384,12 +385,12 @@ END_DEFINE_ACTIVATION(square) * \f] */ BEGIN_DEFINE_ACTIVATION(exponential) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { act.value->exp2(*act.value); return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->expDerivative(*act.value); return Status(); } @@ -402,7 +403,7 @@ END_DEFINE_ACTIVATION(exponential) * \f] */ BEGIN_DEFINE_ACTIVATION(log) -Status forward(Argument& act) { +Status __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -415,7 +416,7 @@ Status forward(Argument& act) { return Status(); } -Status backward(Argument& act) { +Status __must_check backward(Argument& act) { act.grad->dotDiv(*act.grad, *act.in); return Status(); } diff --git a/paddle/gserver/activations/ActivationFunction.h b/paddle/gserver/activations/ActivationFunction.h index ad395ac28da7d9..737df2219dd36f 100644 --- a/paddle/gserver/activations/ActivationFunction.h +++ b/paddle/gserver/activations/ActivationFunction.h @@ -49,7 +49,7 @@ class ActivationFunction { * * Usually, act is Layer::output_ */ - virtual Status forward(Argument& act) = 0; + virtual Status __must_check forward(Argument& act) = 0; /** * @brief Backward propagaion @@ -58,7 +58,7 @@ class ActivationFunction { * - Before calling backward(), act.grad = dE / dy, where E is the error/cost * - After backward() returns, act.grad = dE / dx = (dE/dy) * (dy/dx) */ - virtual Status backward(Argument& act) = 0; + virtual Status __must_check backward(Argument& act) = 0; virtual const std::string& getName() const = 0; }; diff --git a/paddle/gserver/layers/Layer.cpp b/paddle/gserver/layers/Layer.cpp index 06c936c3aec670..f96070fe6e269b 100644 --- a/paddle/gserver/layers/Layer.cpp +++ b/paddle/gserver/layers/Layer.cpp @@ -336,7 +336,7 @@ void Layer::showOutputStats() { void Layer::forwardActivation() { /* activation */ auto status = activation_->forward(output_); - CHECK(status.isOK()) << status.what(); + status.check(); /* dropout */ if (config_.drop_rate() > 0) { @@ -375,7 +375,7 @@ void Layer::backwardActivation() { } auto status = activation_->backward(output_); - CHECK(status.isOK()) << status.what(); + status.check(); } void Layer::forwardDropOut() { diff --git a/paddle/gserver/layers/MDLstmLayer.cpp b/paddle/gserver/layers/MDLstmLayer.cpp index fb41af56319549..88d934d782b549 100644 --- a/paddle/gserver/layers/MDLstmLayer.cpp +++ b/paddle/gserver/layers/MDLstmLayer.cpp @@ -506,9 +506,12 @@ void MDLstmLayer::forwardGate2OutputSequence(int start, *frameState_[start + preOffsetV[i]].value, *checkFgOneDim, 1.0, 1.0); } } - activationGate_->forward(frameInputGate_[idxCurr]); - activationGate_->forward(frameForgetGate_[idxCurr]); - activation_->forward(frameInputNode_[idxCurr]); + auto status = activationGate_->forward(frameInputGate_[idxCurr]); + status.check(); + status = activationGate_->forward(frameForgetGate_[idxCurr]); + status.check(); + status = activation_->forward(frameInputNode_[idxCurr]); + status.check(); frameState_[idxCurr].value->zeroMem(); for (int i = 0; i < numDims_; i++) { @@ -530,10 +533,12 @@ void MDLstmLayer::forwardGate2OutputSequence(int start, frameOutputGate_[idxCurr].value->addDotMul( *frameState_[idxCurr].value, *checkOg_, 1.0, 1.0); - activationGate_->forward(frameOutputGate_[idxCurr]); + status = activationGate_->forward(frameOutputGate_[idxCurr]); + status.check(); framePreOutput_[idxCurr].value->copyFrom(*(frameState_[idxCurr].value)); - activationState_->forward(framePreOutput_[idxCurr]); + status = activationState_->forward(framePreOutput_[idxCurr]); + status.check(); frameOutput_[idxCurr].value->dotMul(*framePreOutput_[idxCurr].value, *frameOutputGate_[idxCurr].value); @@ -640,12 +645,12 @@ void MDLstmLayer::backwardGate2OutputSequence(int start, framePreOutput_[idxCurr].grad->dotMul(*frameOutput_[idxCurr].grad, *frameOutputGate_[idxCurr].value); - activationState_->backward(framePreOutput_[idxCurr]); + activationState_->backward(framePreOutput_[idxCurr]).check(); frameState_[idxCurr].grad->copyFrom(*(framePreOutput_[idxCurr].grad)); frameOutputGate_[idxCurr].grad->dotMul(*frameOutput_[idxCurr].grad, *framePreOutput_[idxCurr].value); - activationGate_->backward(frameOutputGate_[idxCurr]); + activationGate_->backward(frameOutputGate_[idxCurr]).check(); frameState_[idxCurr].grad->addDotMul( *frameOutputGate_[idxCurr].grad, *checkOg_, 1.0, 1.0); @@ -702,9 +707,9 @@ void MDLstmLayer::backwardGate2OutputSequence(int start, } } - activationGate_->backward(frameInputGate_[idxCurr]); - activationGate_->backward(frameForgetGate_[idxCurr]); - activation_->backward(frameInputNode_[idxCurr]); + activationGate_->backward(frameInputGate_[idxCurr]).check(); + activationGate_->backward(frameForgetGate_[idxCurr]).check(); + activation_->backward(frameInputNode_[idxCurr]).check(); if (bias_->getWGrad()) { for (int i = 0; i < numDims_; i++) { diff --git a/paddle/gserver/layers/NCELayer.cpp b/paddle/gserver/layers/NCELayer.cpp index 5ab765247f63df..3542e739df8d03 100644 --- a/paddle/gserver/layers/NCELayer.cpp +++ b/paddle/gserver/layers/NCELayer.cpp @@ -193,7 +193,8 @@ class NCELayer : public Layer { forwardOneInput(l); } - activation_->forward(sampleOut_); + auto status = activation_->forward(sampleOut_); + status.check(); forwardCost(); } @@ -207,7 +208,8 @@ class NCELayer : public Layer { backwardCost(); - activation_->backward(sampleOut_); + auto status = activation_->backward(sampleOut_); + status.check(); if (biases_->getWGrad()) { backwardBias(callback); diff --git a/paddle/gserver/layers/RecurrentLayer.cpp b/paddle/gserver/layers/RecurrentLayer.cpp index 55e0fdfb9048c0..b843fa1265cf3c 100644 --- a/paddle/gserver/layers/RecurrentLayer.cpp +++ b/paddle/gserver/layers/RecurrentLayer.cpp @@ -217,21 +217,22 @@ void RecurrentLayer::forwardOneSequence(int start, int length) { if (prevOutput_) { frameOutput_[start].value->mul(*prevOutput_, *weight_->getW(), 1, 1); } - activation_->forward(frameOutput_[start]); + activation_->forward(frameOutput_[start]).check(); + for (int i = 1; i < length; ++i) { frameOutput_[start + i].value->mul( *frameOutput_[start + i - 1].value, *weight_->getW(), 1, 1); - activation_->forward(frameOutput_[start + i]); + activation_->forward(frameOutput_[start + i]).check(); } if (prevOutput_) { prevOutput_->assign(*frameOutput_[start + length - 1].value); } } else { - activation_->forward(frameOutput_[start + length - 1]); + activation_->forward(frameOutput_[start + length - 1]).check(); for (int i = length - 2; i >= 0; --i) { frameOutput_[start + i].value->mul( *frameOutput_[start + i + 1].value, *weight_->getW(), 1, 1); - activation_->forward(frameOutput_[start + i]); + activation_->forward(frameOutput_[start + i]).check(); } } } @@ -280,11 +281,11 @@ void RecurrentLayer::backwardOneSequence(int start, int length) { MatrixPtr weightT = weight_->getW()->getTranspose(); if (!reversed_) { for (int i = length - 1; i > 0; --i) { - activation_->backward(frameOutput_[start + i]); + activation_->backward(frameOutput_[start + i]).check(); frameOutput_[start + i - 1].grad->mul( *frameOutput_[start + i].grad, *weightT, 1, 1); } - activation_->backward(frameOutput_[start]); + activation_->backward(frameOutput_[start]).check(); if (weight_->getWGrad()) { weight_->getWGrad()->mul( *output_.value->subMatrix(start, length - 1)->getTranspose(), @@ -294,11 +295,11 @@ void RecurrentLayer::backwardOneSequence(int start, int length) { } } else { for (int i = 0; i < length - 1; ++i) { - activation_->backward(frameOutput_[start + i]); + activation_->backward(frameOutput_[start + i]).check(); frameOutput_[start + i + 1].grad->mul( *frameOutput_[start + i].grad, *weightT, 1, 1); } - activation_->backward(frameOutput_[start + length - 1]); + activation_->backward(frameOutput_[start + length - 1]).check(); if (weight_->getWGrad()) { weight_->getWGrad()->mul( *output_.value->subMatrix(start + 1, length - 1)->getTranspose(), @@ -333,7 +334,7 @@ void RecurrentLayer::forwardBatch(int batchSize, } Argument arg; arg.value = batch2; - activation_->forward(arg); + activation_->forward(arg).check(); } } batchValue_->copyBackSeq(*output_.value); @@ -363,7 +364,7 @@ void RecurrentLayer::backwardBatch(int batchSize, Argument arg; arg.value = batch1; arg.grad = batch2; - activation_->backward(arg); + activation_->backward(arg).check(); if (n != 0) { batch1 = batchGrad_->getBatchValue(n - 1, batch2->getHeight()); diff --git a/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp b/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp index 5eacff6b714399..d9a91de8a6f4da 100644 --- a/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp +++ b/paddle/gserver/layers/SelectiveFullyConnectedLayer.cpp @@ -192,7 +192,8 @@ void SelectiveFullyConnectedLayer::forward(PassType passType) { nnz, /*trans=*/false, /*useGpu=*/useGpu_); - activation_->forward(arg); + //! TODO(yuyang18): Why we cannot invoke forwardActivation here? + activation_->forward(arg).check(); } else /* train and test in train, not generating */ { // during training, this layer output value is *Matrix*, which is input of // eg. multi-class-cross-entropy diff --git a/paddle/gserver/tests/test_WarpCTCLayer.cpp b/paddle/gserver/tests/test_WarpCTCLayer.cpp index 23ae95852e8421..55427e2f12fd7b 100644 --- a/paddle/gserver/tests/test_WarpCTCLayer.cpp +++ b/paddle/gserver/tests/test_WarpCTCLayer.cpp @@ -148,11 +148,11 @@ LayerPtr createCTCLayer(string name, ActivationFunction* softmaxActivation = ActivationFunction::create("softmax"); - softmaxActivation->forward(dataLayer->getOutput()); + softmaxActivation->forward(dataLayer->getOutput()).check(); layer->forward(PASS_GC); layer->backward(); - softmaxActivation->backward(dataLayer->getOutput()); + softmaxActivation->backward(dataLayer->getOutput()).check(); return layer; } diff --git a/paddle/utils/Compiler.h b/paddle/utils/Compiler.h index e69de29bb2d1d6..22812e83987d8d 100644 --- a/paddle/utils/Compiler.h +++ b/paddle/utils/Compiler.h @@ -0,0 +1,25 @@ +/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved. +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 + +#ifdef __GNUC__ +#define GCC_VERSION \ + (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) +#else +#define GCC_VERSION +#endif + +#if GCC_VERSION >= 30400 +#define __must_check __attribute__((warn_unused_result)) +#else +#define __must_check +#endif diff --git a/paddle/utils/Status.h b/paddle/utils/Status.h index cb66e4b225e92c..26329f8d19bae6 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Status.h @@ -14,9 +14,11 @@ limitations under the License. */ #pragma once +#include #include #include #include +#include "Compiler.h" namespace paddle { @@ -29,8 +31,55 @@ namespace paddle { * There are two styles to return status in Paddle. * * 1. Return Status + * When method return a status, the return must use `__must_check` attribute. + * Example as below. + * @code{cpp} + * Status __must_check foo(); * + * Status __must_check bar() { + * // do something. + * Status s = foo(); // invoke other method return status. + * if (!s.isOK()) return s; + * // do something else. + * return Status(); + * } + * @endcode{cpp} * + * 2. Return by parameter. + * It is another way to return a status, by using a pointer parameter. + * Example as below. + * + * @code{cpp} + * Status bar(); + * + * int foo(Status* status) { + * // Do something. + * Status s = bar(); + * if (!s.isOK()) { + * *status = s; + * return 0; + * } + * // Do something else. + * if (someInternalErrorHappend) { + * status->setByPrintf("Some dimension is too large, %d", dimension); + * return 0; + * } + * // End of method. + * return someValue; + * } + * + * Status foobar() { + * Status s; + * // do something. + * foo(&s); + * if (!s.isOK()) return s; + * } + * @endcode{cpp} + * + * + * Currently there is a helper method 'check' in status, because Paddle always + * use log(FATAL) or CHECK to make program exit before. When we clean all + * log(FATAL) and CHECK in Paddle, 'check' method will be removed. */ class Status final : public std::exception { public: @@ -92,6 +141,13 @@ class Status final : public std::exception { */ inline bool isOK() const noexcept { return errMsg_ == nullptr; } + /** + * @brief check this status by glog. + * @note It is a temp method used during cleaning Paddle code. It will be + * removed later. + */ + inline void check() const { CHECK(isOK()) << what(); } + private: std::shared_ptr errMsg_; }; From 8605544c0b0cbc5ad43d86a71402f3f4075b48e3 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 12:49:00 +0800 Subject: [PATCH 07/13] Add some comments to compiler.h --- paddle/utils/Compiler.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/paddle/utils/Compiler.h b/paddle/utils/Compiler.h index 22812e83987d8d..cebca5a2a37661 100644 --- a/paddle/utils/Compiler.h +++ b/paddle/utils/Compiler.h @@ -10,7 +10,10 @@ See the License for the specific language governing permissions and limitations under the License. */ #pragma once - +/** + * This header defines some useful attribute by each compiler. It is the + * abstract layer of compilers. + */ #ifdef __GNUC__ #define GCC_VERSION \ (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) @@ -18,6 +21,11 @@ limitations under the License. */ #define GCC_VERSION #endif +/** + * __must_check macro. It make the function's return value must be used, + * otherwise it will raise a compile warning. And also Paddle treat all compile + * warnings as errors. + */ #if GCC_VERSION >= 30400 #define __must_check __attribute__((warn_unused_result)) #else From ec790e1050b52b09a182eed95fc030c7879e5012 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 13:24:02 +0800 Subject: [PATCH 08/13] Rename Status => Error. * Also make ErrorF as a global method. --- .../activations/ActivationFunction.cpp | 110 +++++++++--------- .../gserver/activations/ActivationFunction.h | 6 +- paddle/gserver/layers/Layer.cpp | 2 +- paddle/utils/{Status.h => Error.h} | 92 ++++++++------- paddle/utils/tests/CMakeLists.txt | 2 +- .../tests/{test_Status.cpp => test_Error.cpp} | 14 +-- 6 files changed, 114 insertions(+), 112 deletions(-) rename paddle/utils/{Status.h => Error.h} (70%) rename paddle/utils/tests/{test_Status.cpp => test_Error.cpp} (76%) diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index 666c2e01c8bdaf..f1f96fc67d04f4 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -69,13 +69,13 @@ static ClassRegistrar gActivationRegistrar; class IdentityActivation : public ActivationFunction { public: static const std::string name; - Status __must_check forward(Argument& act) { + Error __must_check forward(Argument& act) { (void)act; - return Status(); + return Error(); } - Status __must_check backward(Argument& act) { + Error __must_check backward(Argument& act) { (void)act; - return Status(); + return Error(); } const std::string& getName() const { return name; } }; @@ -92,13 +92,13 @@ static InitFunction __reg_activation__identity([] { * \f] */ BEGIN_DEFINE_ACTIVATION(sigmoid) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->sigmoid(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->sigmoidDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(sigmoid) @@ -115,12 +115,12 @@ MatrixPtr sftMaxDot_; MatrixPtr one_; public: -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->softmax(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { MatrixPtr outputV = act.value; MatrixPtr outputG = act.grad; @@ -152,7 +152,7 @@ Status __must_check backward(Argument& act) { act.grad->softmaxDerivative(*act.value, *sftMaxSum_); } - return Status(); + return Error(); } END_DEFINE_ACTIVATION(softmax) @@ -167,9 +167,9 @@ ACTIVATION_CLASS_NAME(softmax) softmax_; Argument argument_; public: -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { if (act.value->getWidth() != 1UL) { - return Status( + return ErrorF( "Input width for each timestep of sequence softmax should be 1"); } @@ -188,12 +188,12 @@ Status __must_check forward(Argument& act) { auto starts = act.sequenceStartPositions->getVector(useGpu(act.deviceId)); act.value->sequenceSoftmax(*act.value, *starts); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { if (act.value->getWidth() != 1UL) { - return Status( + return ErrorF( "Input width for each timestep of sequence softmax should be 1"); } @@ -207,10 +207,10 @@ Status __must_check backward(Argument& act) { argument_.value->setData(act.value->getData() + offset, 1UL, size); argument_.grad->setData(act.grad->getData() + offset, 1UL, size); - Status status = softmax_.backward(argument_); + Error status = softmax_.backward(argument_); if (!status.isOK()) return status; } - return Status(); + return Error(); } END_DEFINE_ACTIVATION(sequence_softmax) @@ -225,14 +225,14 @@ END_DEFINE_ACTIVATION(sequence_softmax) * 0 otherwise. */ BEGIN_DEFINE_ACTIVATION(relu) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->relu(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->reluDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(relu) @@ -250,14 +250,14 @@ END_DEFINE_ACTIVATION(relu) * TODO(yuyang18): Remove magic number 24 or make it configuable. */ BEGIN_DEFINE_ACTIVATION(brelu) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->brelu(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->breluDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(brelu) @@ -268,14 +268,14 @@ END_DEFINE_ACTIVATION(brelu) * \f] */ BEGIN_DEFINE_ACTIVATION(tanh) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->tanh(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->tanhDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(tanh) @@ -291,14 +291,14 @@ real a, b; public: ACTIVATION_CLASS_NAME(stanh)() : a(1.7159), b(2. / 3.) {} -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->scaledTanh(*act.value, a, b); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->scaledTanhDerivative(*act.value, a, b); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(stanh) @@ -309,14 +309,14 @@ END_DEFINE_ACTIVATION(stanh) * \f] */ BEGIN_DEFINE_ACTIVATION(softrelu) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->softrelu(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->softreluDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(softrelu) @@ -333,7 +333,7 @@ END_DEFINE_ACTIVATION(softrelu) * 0 if z=0 */ BEGIN_DEFINE_ACTIVATION(abs) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -343,12 +343,12 @@ Status __must_check forward(Argument& act) { act.in->copyFrom(*act.value); act.value->abs2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->absDerivative(*act.in); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(abs) @@ -359,7 +359,7 @@ END_DEFINE_ACTIVATION(abs) * \f] */ BEGIN_DEFINE_ACTIVATION(square) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -369,12 +369,12 @@ Status __must_check forward(Argument& act) { act.in->copyFrom(*act.value); act.value->square2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->squareDerivative(*act.in); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(square) @@ -385,14 +385,14 @@ END_DEFINE_ACTIVATION(square) * \f] */ BEGIN_DEFINE_ACTIVATION(exponential) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { act.value->exp2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->expDerivative(*act.value); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(exponential) @@ -403,7 +403,7 @@ END_DEFINE_ACTIVATION(exponential) * \f] */ BEGIN_DEFINE_ACTIVATION(log) -Status __must_check forward(Argument& act) { +Error __must_check forward(Argument& act) { SetDevice device(act.deviceId); Matrix::resizeOrCreate(act.in, act.value->getHeight(), @@ -413,12 +413,12 @@ Status __must_check forward(Argument& act) { act.in->copyFrom(*act.value); act.value->log2(*act.value); - return Status(); + return Error(); } -Status __must_check backward(Argument& act) { +Error __must_check backward(Argument& act) { act.grad->dotDiv(*act.grad, *act.in); - return Status(); + return Error(); } END_DEFINE_ACTIVATION(log) diff --git a/paddle/gserver/activations/ActivationFunction.h b/paddle/gserver/activations/ActivationFunction.h index 737df2219dd36f..f208224e304a79 100644 --- a/paddle/gserver/activations/ActivationFunction.h +++ b/paddle/gserver/activations/ActivationFunction.h @@ -15,7 +15,7 @@ limitations under the License. */ #pragma once #include #include -#include "paddle/utils/Status.h" +#include "paddle/utils/Error.h" namespace paddle { @@ -49,7 +49,7 @@ class ActivationFunction { * * Usually, act is Layer::output_ */ - virtual Status __must_check forward(Argument& act) = 0; + virtual Error __must_check forward(Argument& act) = 0; /** * @brief Backward propagaion @@ -58,7 +58,7 @@ class ActivationFunction { * - Before calling backward(), act.grad = dE / dy, where E is the error/cost * - After backward() returns, act.grad = dE / dx = (dE/dy) * (dy/dx) */ - virtual Status __must_check backward(Argument& act) = 0; + virtual Error __must_check backward(Argument& act) = 0; virtual const std::string& getName() const = 0; }; diff --git a/paddle/gserver/layers/Layer.cpp b/paddle/gserver/layers/Layer.cpp index f96070fe6e269b..f76d41ad3e8a3b 100644 --- a/paddle/gserver/layers/Layer.cpp +++ b/paddle/gserver/layers/Layer.cpp @@ -15,8 +15,8 @@ limitations under the License. */ #include "paddle/utils/Util.h" #include "paddle/math/SparseMatrix.h" +#include "paddle/utils/Error.h" #include "paddle/utils/Logging.h" -#include "paddle/utils/Status.h" #include "AddtoLayer.h" #include "CRFLayer.h" diff --git a/paddle/utils/Status.h b/paddle/utils/Error.h similarity index 70% rename from paddle/utils/Status.h rename to paddle/utils/Error.h index 26329f8d19bae6..f1597f93d2579a 100644 --- a/paddle/utils/Status.h +++ b/paddle/utils/Error.h @@ -34,9 +34,9 @@ namespace paddle { * When method return a status, the return must use `__must_check` attribute. * Example as below. * @code{cpp} - * Status __must_check foo(); + * Error __must_check foo(); * - * Status __must_check bar() { + * Error __must_check bar() { * // do something. * Status s = foo(); // invoke other method return status. * if (!s.isOK()) return s; @@ -50,9 +50,9 @@ namespace paddle { * Example as below. * * @code{cpp} - * Status bar(); + * Error bar(); * - * int foo(Status* status) { + * int foo(Error* status) { * // Do something. * Status s = bar(); * if (!s.isOK()) { @@ -61,15 +61,15 @@ namespace paddle { * } * // Do something else. * if (someInternalErrorHappend) { - * status->setByPrintf("Some dimension is too large, %d", dimension); + * *status = ErrorF("Some dimension is too large, %d", dimension); * return 0; * } * // End of method. * return someValue; * } * - * Status foobar() { - * Status s; + * Error foobar() { + * Error s; * // do something. * foo(&s); * if (!s.isOK()) return s; @@ -81,48 +81,12 @@ namespace paddle { * use log(FATAL) or CHECK to make program exit before. When we clean all * log(FATAL) and CHECK in Paddle, 'check' method will be removed. */ -class Status final : public std::exception { +class Error final : public std::exception { public: /** * Default Status. OK */ - Status() noexcept {} - - /** - * @brief Create Status with error message - * @param msg - */ - explicit Status(const std::string& msg) : errMsg_(new std::string(msg)) {} - - /** - * @brief set a error message for status. - * @param msg - */ - inline void set(const std::string& msg) noexcept { - errMsg_.reset(new std::string(msg)); - } - - /** - * @brief set a error message for status. Use C style printf - * @param fmt - */ - template - inline void setByPrintf(const char* fmt, ARGS... args) noexcept { - constexpr size_t kBufferSize = 1024; // 1KB buffer - char buffer[kBufferSize]; - snprintf(buffer, kBufferSize, fmt, args...); - errMsg_.reset(new std::string(buffer)); - } - - /** - * create a error status by C style printf. - */ - template - inline static Status printf(const char* fmt, ARGS... args) noexcept { - Status s; - s.setByPrintf(fmt, args...); - return s; - } + Error() noexcept {} /** * @brief what will return the error message. If status is OK, return nullptr. @@ -148,8 +112,46 @@ class Status final : public std::exception { */ inline void check() const { CHECK(isOK()) << what(); } + /** + * friend method to create Error. + */ + template + friend Error __must_check ErrorF(const char* fmt, ARGS... args); + private: std::shared_ptr errMsg_; }; +/** + * ErrorF will create an Error by printf syntax. + * + * Specialize this method because clang will give a warning when use printf(fmt) + * without arguments. + */ +template <> +inline Error __must_check ErrorF(const char* msg) { + Error e; + e.errMsg_.reset(new std::string(msg)); + return e; +} + +/** + * ErrorF will create an Error by printf syntax. + * + * Examples: + * @code{cpp} + * auto err = ErrorF("SomeError"); + * auto err2 = ErrorF("SomeErrorWithParameter %f %d", real_val, int_val); + * @endcode{cpp} + */ +template +inline Error __must_check ErrorF(const char* fmt, ARGS... args) { + constexpr size_t kBufferSize = 1024; + char buffer[kBufferSize]; + snprintf(buffer, kBufferSize, fmt, args...); + Error e; + e.errMsg_.reset(new std::string(buffer)); + return e; +} + } // namespace paddle diff --git a/paddle/utils/tests/CMakeLists.txt b/paddle/utils/tests/CMakeLists.txt index a1cc32668d5100..aa923b35537775 100644 --- a/paddle/utils/tests/CMakeLists.txt +++ b/paddle/utils/tests/CMakeLists.txt @@ -4,7 +4,7 @@ add_simple_unittest(test_CustomStackTrace) add_simple_unittest(test_ThreadBarrier) add_simple_unittest(test_SpinLock) add_simple_unittest(test_SIMDFlags) -add_simple_unittest(test_Status) +add_simple_unittest(test_Error) add_executable( test_CustomStackTracePrint diff --git a/paddle/utils/tests/test_Status.cpp b/paddle/utils/tests/test_Error.cpp similarity index 76% rename from paddle/utils/tests/test_Status.cpp rename to paddle/utils/tests/test_Error.cpp index 04cef095792c73..96115f7053aec5 100644 --- a/paddle/utils/tests/test_Status.cpp +++ b/paddle/utils/tests/test_Error.cpp @@ -12,23 +12,23 @@ 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/utils/Status.h" +#include "paddle/utils/Error.h" #include TEST(Status, testAll) { - paddle::Status status; + paddle::Error status; ASSERT_TRUE(status.isOK()); - status.set("I'm the error"); + status = paddle::ErrorF("I'm the error"); ASSERT_FALSE(status.isOK()); ASSERT_STREQ("I'm the error", status.what()); - paddle::Status status2("error2"); - ASSERT_FALSE(status2.isOK()); - ASSERT_STREQ("error2", status2.what()); + status = paddle::ErrorF("error2"); + ASSERT_FALSE(status.isOK()); + ASSERT_STREQ("error2", status.what()); int i = 3; - auto status3 = paddle::Status::printf("error%d", i); + auto status3 = paddle::ErrorF("error%d", i); ASSERT_FALSE(status3.isOK()); ASSERT_STREQ("error3", status3.what()); } From 699d18f11701aae1efe72c8bf6edc50723445050 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 13:34:20 +0800 Subject: [PATCH 09/13] Change unittest variable name --- paddle/utils/tests/test_Error.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/paddle/utils/tests/test_Error.cpp b/paddle/utils/tests/test_Error.cpp index 96115f7053aec5..e8643de9d2dccf 100644 --- a/paddle/utils/tests/test_Error.cpp +++ b/paddle/utils/tests/test_Error.cpp @@ -16,19 +16,19 @@ limitations under the License. */ #include -TEST(Status, testAll) { - paddle::Error status; - ASSERT_TRUE(status.isOK()); - status = paddle::ErrorF("I'm the error"); - ASSERT_FALSE(status.isOK()); - ASSERT_STREQ("I'm the error", status.what()); +TEST(Error, testAll) { + paddle::Error error; + ASSERT_TRUE(error.isOK()); + error = paddle::ErrorF("I'm the error"); + ASSERT_FALSE(error.isOK()); + ASSERT_STREQ("I'm the error", error.what()); - status = paddle::ErrorF("error2"); - ASSERT_FALSE(status.isOK()); - ASSERT_STREQ("error2", status.what()); + error = paddle::ErrorF("error2"); + ASSERT_FALSE(error.isOK()); + ASSERT_STREQ("error2", error.what()); int i = 3; - auto status3 = paddle::ErrorF("error%d", i); - ASSERT_FALSE(status3.isOK()); - ASSERT_STREQ("error3", status3.what()); + auto error3 = paddle::ErrorF("error%d", i); + ASSERT_FALSE(error3.isOK()); + ASSERT_STREQ("error3", error3.what()); } From 5a15c70e167a83e6b06686e3152ca9b30ed7800e Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Wed, 18 Jan 2017 20:27:52 +0800 Subject: [PATCH 10/13] Make Error interface cleaner --- .../activations/ActivationFunction.cpp | 6 +- paddle/utils/Error.h | 92 +++++++------------ paddle/utils/tests/test_Error.cpp | 20 ++-- 3 files changed, 45 insertions(+), 73 deletions(-) diff --git a/paddle/gserver/activations/ActivationFunction.cpp b/paddle/gserver/activations/ActivationFunction.cpp index f1f96fc67d04f4..c541b72e104bf2 100644 --- a/paddle/gserver/activations/ActivationFunction.cpp +++ b/paddle/gserver/activations/ActivationFunction.cpp @@ -169,7 +169,7 @@ Argument argument_; public: Error __must_check forward(Argument& act) { if (act.value->getWidth() != 1UL) { - return ErrorF( + return Error( "Input width for each timestep of sequence softmax should be 1"); } @@ -193,7 +193,7 @@ Error __must_check forward(Argument& act) { Error __must_check backward(Argument& act) { if (act.value->getWidth() != 1UL) { - return ErrorF( + return Error( "Input width for each timestep of sequence softmax should be 1"); } @@ -208,7 +208,7 @@ Error __must_check backward(Argument& act) { argument_.grad->setData(act.grad->getData() + offset, 1UL, size); Error status = softmax_.backward(argument_); - if (!status.isOK()) return status; + if (!status) return status; } return Error(); } diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index f1597f93d2579a..a8de56b9808b81 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -23,14 +23,12 @@ limitations under the License. */ namespace paddle { /** - * Status is Paddle error code. It only contain a std::string as error message. - * Although Status inherits the std::exception, but do not throw it except you - * know what you are doing. + * Error is Paddle error code. It only contain a std::string as error message. * * - * There are two styles to return status in Paddle. + * There are two styles to return error in Paddle. * - * 1. Return Status + * 1. Return Error * When method return a status, the return must use `__must_check` attribute. * Example as below. * @code{cpp} @@ -39,29 +37,29 @@ namespace paddle { * Error __must_check bar() { * // do something. * Status s = foo(); // invoke other method return status. - * if (!s.isOK()) return s; + * if (!s) return s; * // do something else. * return Status(); * } * @endcode{cpp} * * 2. Return by parameter. - * It is another way to return a status, by using a pointer parameter. + * It is another way to return an error, by using a pointer parameter. * Example as below. * * @code{cpp} * Error bar(); * - * int foo(Error* status) { + * int foo(Error* error) { * // Do something. - * Status s = bar(); - * if (!s.isOK()) { - * *status = s; + * Error s = bar(); + * if (!s) { + * *error = s; * return 0; * } * // Do something else. * if (someInternalErrorHappend) { - * *status = ErrorF("Some dimension is too large, %d", dimension); + * *error = Error("Some dimension is too large, %d", dimension); * return 0; * } * // End of method. @@ -72,7 +70,7 @@ namespace paddle { * Error s; * // do something. * foo(&s); - * if (!s.isOK()) return s; + * if (!s) return s; * } * @endcode{cpp} * @@ -81,17 +79,31 @@ namespace paddle { * use log(FATAL) or CHECK to make program exit before. When we clean all * log(FATAL) and CHECK in Paddle, 'check' method will be removed. */ -class Error final : public std::exception { +class Error final { public: /** * Default Status. OK */ - Error() noexcept {} + inline Error() {} /** - * @brief what will return the error message. If status is OK, return nullptr. + * @brief Create an Error use printf syntax. */ - const char* what() const noexcept override { + inline explicit Error(const char* fmt, ...) { + va_list ap; + va_start(ap, fmt); + constexpr size_t kBufferSize = 1024; + this->errMsg_.reset(new std::string(kBufferSize, 0)); + auto sz = vsnprintf(&(*errMsg_)[0], kBufferSize, fmt, ap); + this->errMsg_->resize(sz); + this->errMsg_->shrink_to_fit(); + va_end(ap); + } + + /** + * @brief what will return the error message. If no error, return nullptr. + */ + inline const char* msg() const { if (errMsg_) { return errMsg_->data(); } else { @@ -100,58 +112,18 @@ class Error final : public std::exception { } /** - * @brief isOK - * @return true if OK. + * @brief operator bool, return True if there is no error. */ - inline bool isOK() const noexcept { return errMsg_ == nullptr; } - + inline operator bool() const { return !errMsg_; } /** * @brief check this status by glog. * @note It is a temp method used during cleaning Paddle code. It will be * removed later. */ - inline void check() const { CHECK(isOK()) << what(); } - - /** - * friend method to create Error. - */ - template - friend Error __must_check ErrorF(const char* fmt, ARGS... args); + inline void check() const { CHECK(*this) << msg(); } private: std::shared_ptr errMsg_; }; -/** - * ErrorF will create an Error by printf syntax. - * - * Specialize this method because clang will give a warning when use printf(fmt) - * without arguments. - */ -template <> -inline Error __must_check ErrorF(const char* msg) { - Error e; - e.errMsg_.reset(new std::string(msg)); - return e; -} - -/** - * ErrorF will create an Error by printf syntax. - * - * Examples: - * @code{cpp} - * auto err = ErrorF("SomeError"); - * auto err2 = ErrorF("SomeErrorWithParameter %f %d", real_val, int_val); - * @endcode{cpp} - */ -template -inline Error __must_check ErrorF(const char* fmt, ARGS... args) { - constexpr size_t kBufferSize = 1024; - char buffer[kBufferSize]; - snprintf(buffer, kBufferSize, fmt, args...); - Error e; - e.errMsg_.reset(new std::string(buffer)); - return e; -} - } // namespace paddle diff --git a/paddle/utils/tests/test_Error.cpp b/paddle/utils/tests/test_Error.cpp index e8643de9d2dccf..85156466e2cafd 100644 --- a/paddle/utils/tests/test_Error.cpp +++ b/paddle/utils/tests/test_Error.cpp @@ -18,17 +18,17 @@ limitations under the License. */ TEST(Error, testAll) { paddle::Error error; - ASSERT_TRUE(error.isOK()); - error = paddle::ErrorF("I'm the error"); - ASSERT_FALSE(error.isOK()); - ASSERT_STREQ("I'm the error", error.what()); + ASSERT_TRUE(error); + error = paddle::Error("I'm the error"); + ASSERT_FALSE(error); + ASSERT_STREQ("I'm the error", error.msg()); - error = paddle::ErrorF("error2"); - ASSERT_FALSE(error.isOK()); - ASSERT_STREQ("error2", error.what()); + error = paddle::Error("error2"); + ASSERT_FALSE(error); + ASSERT_STREQ("error2", error.msg()); int i = 3; - auto error3 = paddle::ErrorF("error%d", i); - ASSERT_FALSE(error3.isOK()); - ASSERT_STREQ("error3", error3.what()); + auto error3 = paddle::Error("error%d", i); + ASSERT_FALSE(error3); + ASSERT_STREQ("error3", error3.msg()); } From a6d4a31deae38f78e24ecaf198e9250927416041 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 19 Jan 2017 10:01:47 +0800 Subject: [PATCH 11/13] Follow comments --- paddle/utils/Error.h | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index a8de56b9808b81..ff11541bbd781f 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -79,33 +79,32 @@ namespace paddle { * use log(FATAL) or CHECK to make program exit before. When we clean all * log(FATAL) and CHECK in Paddle, 'check' method will be removed. */ -class Error final { +class Error { public: /** - * Default Status. OK + * Construct an no-error value. */ - inline Error() {} + Error() {} /** * @brief Create an Error use printf syntax. */ - inline explicit Error(const char* fmt, ...) { + explicit Error(const char* fmt, ...) { va_list ap; va_start(ap, fmt); constexpr size_t kBufferSize = 1024; - this->errMsg_.reset(new std::string(kBufferSize, 0)); - auto sz = vsnprintf(&(*errMsg_)[0], kBufferSize, fmt, ap); - this->errMsg_->resize(sz); - this->errMsg_->shrink_to_fit(); + char buffer[kBufferSize]; + vsnprintf(buffer, kBufferSize, fmt, ap); + this->msg_.reset(new std::string(buffer)); va_end(ap); } /** * @brief what will return the error message. If no error, return nullptr. */ - inline const char* msg() const { - if (errMsg_) { - return errMsg_->data(); + const char* msg() const { + if (msg_) { + return msg_->c_str(); } else { return nullptr; } @@ -114,16 +113,16 @@ class Error final { /** * @brief operator bool, return True if there is no error. */ - inline operator bool() const { return !errMsg_; } + operator bool() const { return !msg_; } /** * @brief check this status by glog. * @note It is a temp method used during cleaning Paddle code. It will be * removed later. */ - inline void check() const { CHECK(*this) << msg(); } + void check() const { CHECK(*this) << msg(); } private: - std::shared_ptr errMsg_; + std::shared_ptr msg_; }; } // namespace paddle From c88dec209f367cb3ac1bd3fe6964e63f7274d975 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 19 Jan 2017 10:08:33 +0800 Subject: [PATCH 12/13] Fix typo --- paddle/utils/Error.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index ff11541bbd781f..6fe7b6ea8884ec 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -82,7 +82,7 @@ namespace paddle { class Error { public: /** - * Construct an no-error value. + * Construct a no-error value. */ Error() {} @@ -100,7 +100,7 @@ class Error { } /** - * @brief what will return the error message. If no error, return nullptr. + * @brief msg will return the error message. If no error, return nullptr. */ const char* msg() const { if (msg_) { @@ -114,6 +114,7 @@ class Error { * @brief operator bool, return True if there is no error. */ operator bool() const { return !msg_; } + /** * @brief check this status by glog. * @note It is a temp method used during cleaning Paddle code. It will be From 843fb2ea32d4f0b2d1f3667545487c8084229819 Mon Sep 17 00:00:00 2001 From: Yu Yang Date: Thu, 19 Jan 2017 15:12:42 +0800 Subject: [PATCH 13/13] Make code more readable --- paddle/utils/Error.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/paddle/utils/Error.h b/paddle/utils/Error.h index 6fe7b6ea8884ec..2b4fbef4e015e7 100644 --- a/paddle/utils/Error.h +++ b/paddle/utils/Error.h @@ -15,6 +15,7 @@ limitations under the License. */ #pragma once #include +#include #include #include #include @@ -113,7 +114,7 @@ class Error { /** * @brief operator bool, return True if there is no error. */ - operator bool() const { return !msg_; } + operator bool() const { return msg_ == nullptr; } /** * @brief check this status by glog.