Use KernelType instead of FunctionBase in Layer #2049
Conversation
* Make paddle::Layer decoupled with FunctionBase.
| | ||
| #include <vector> | ||
| #include "NormLayer.h" | ||
| #include "WithFunction.h" |
There was a problem hiding this comment.
看起来每个Layer都需要 include "WithFunction.h" 直接放到Layer.h里面吧。
There was a problem hiding this comment.
同理BufferArg.h这个好像也是每个Layer中都include了。
There was a problem hiding this comment.
OK, I will fix this ASAP.
There was a problem hiding this comment.
I'd thought that this PR is going to replace function::FunctionBase by std::function. But it is not. Instead, it is a wrapper which hides function::FunctionBase behind a std::function.
I believe that if we don't need a concept, we should remove it or replace it, not hide it.
paddle/function/KernelType.h Outdated
| //! Kernel Function type of Paddle. | ||
| //! Each layer will invoke this KernelType when forward/backward. | ||
| typedef std::function<Error(BufferArgs& inputs, BufferArgs& outputs)> | ||
| KernelType; |
There was a problem hiding this comment.
"kernel" is a polysemy. It could mean OS kernel, Linux kernel, CUDA kernel, etc. Here we need a specific name, e.g., KernelType ==> CUDAKernel.
There was a problem hiding this comment.
CUDA is only for GPU. Perhaps we can use Function or FunctionType.
| See the License for the specific language governing permissions and | ||
| limitations under the License. */ | ||
| | ||
| #include "ContextProjection.h" |
There was a problem hiding this comment.
Either in this PR or in the next one, we need to use full path with #includes. For example,
#include "ContextProjection.h" should we
#include "paddle/gserver/layers/ContextProjection.h"This change should be done together with the change of CMakeLists.txt files to remove some pre-defined C++ include paths.
There was a problem hiding this comment.
I think a cpp can always include the header file in its own directory.
| * The config file api is context_projection. | ||
| */ | ||
| class ContextProjection : public Projection { | ||
| class ContextProjection : public Projection, public WithFunction { |
There was a problem hiding this comment.
There have been many discussions about why we need to avoid C++ multiple inheritances. E.g., http://stackoverflow.com/a/407928/724872. Instead, we should use "composition".
There was a problem hiding this comment.
Currently, WithFunction is a temporary class during refactoring Paddle C++ Core. After we refactor Paddle C++ Core, all Layer and Project should be WithFunction and then Layer and Project will be a subclass of WithFunction.
There was a problem hiding this comment.
Done. I use FunctionList instead of WithFunction, make FunctionList as a member of Layer and Projection.
paddle/gserver/layers/WithFunction.h Outdated
| */ | ||
| class WithFunction { | ||
| protected: | ||
| std::vector<function::KernelType> forward_; |
There was a problem hiding this comment.
If all classes derived from Projection need forward_ and backward_, we should define them in Projection; instead of here.
| const std::string &name, | ||
| const FuncConfig &config, | ||
| bool useGPU) { | ||
| std::shared_ptr<FunctionBase> func; |
There was a problem hiding this comment.
In #2050 which was claimed to be fixed with this PR, it says that we shoud replace FunctionBase with std::function. But here it shows that FunctionBase is not replaced at all. We just wrap it by a std::function. I believe that if a concept is not necessary, we should remove it, instead of hide it.
There was a problem hiding this comment.
I want to replace FunctionBase by plain function step by step. The first step is to use std::function to hide it.
The demo code of register function is here
reg_cpu_function(cos, [](const BufferArgs& ins, const BufferArgs& outs, real scale){ ... });
| See the License for the specific language governing permissions and | ||
| limitations under the License. */ | ||
| | ||
| #include "ContextProjection.h" |
There was a problem hiding this comment.
I think a cpp can always include the header file in its own directory.
paddle/gserver/layers/WithFunction.h Outdated
| */ | ||
| class WithFunction { | ||
| protected: | ||
| std::vector<function::KernelType> forward_; |
There was a problem hiding this comment.
Why do we need a vector here?
There was a problem hiding this comment.
There may be many functions used in one layer's forward/backward. The layer could be composed of functions.
| | ||
| #include <vector> | ||
| #include "NormLayer.h" | ||
| #include "WithFunction.h" |
paddle/function/KernelType.h Outdated
| //! Kernel Function type of Paddle. | ||
| //! Each layer will invoke this KernelType when forward/backward. | ||
| typedef std::function<Error(BufferArgs& inputs, BufferArgs& outputs)> | ||
| KernelType; |
There was a problem hiding this comment.
CUDA is only for GPU. Perhaps we can use Function or FunctionType.
| 感谢您给PaddlePaddle贡献代码。由于Paddle V1/V2版本已不再维护，相关代码也已从develop分支上删除，因此关闭您的PR，欢迎您向Paddle最新版-Fluid贡献代码。 |
5 participants
paddle::Functionas plain Functions #2050