Enhanced is_empty_op for our seq2seq model #10704
Conversation
paddle/fluid/operators/is_empty_op.h
Outdated
| public: | ||
| void Compute(const framework::ExecutionContext& context) const override { | ||
| // get input | ||
| auto* input_tensor = context.Input<framework::LoDTensor>(kInput); |
There was a problem hiding this comment.
Why not just context.Input<framework::LoDTensor>("X") here.
There was a problem hiding this comment.
It is the original design of the op, I'm not sure about the reason either. Maybe due to the requirement of generality? See line 21 of original code is_empty_op.cc
| "Input(X) of IsEmptyOp should not be null."); | ||
| PADDLE_ENFORCE(ctx->HasOutput("Out"), | ||
| "Output(Out) of IsEmptyOp should not be null."); | ||
| } |
There was a problem hiding this comment.
Better to set dims for 'Out' here.
There was a problem hiding this comment.
SetDim is defined only for LoDTensor and SelectedRows, but this op is for general tensors. Please have a look at
Paddle/paddle/fluid/framework/operator.cc
Line 485 in 868bdc9
There was a problem hiding this comment.
Currently, we only use LoDTensor. Normal tensor is LoDTensor without LoD.
There was a problem hiding this comment.
The point is that SetDim doesn't work for normal tensors. If you want use SetDim for a normal tensor you have to change the implementation of that function.
paddle/fluid/operators/is_empty_op.h
Outdated
| // get output | ||
| auto* output_tensor = context.Output<framework::LoDTensor>(kOutput); | ||
|
|
||
| output_tensor->Resize({1}); |
There was a problem hiding this comment.
Please specify the dims of 'Out' in InferShape.
| paddle::operators::IsEmptyOpProtoMaker); | ||
| namespace ops = paddle::operators; | ||
|
|
||
| REGISTER_OPERATOR(is_empty, ops::IsEmptyOp, ops::IsEmptyOpMaker); |
There was a problem hiding this comment.
This operator has no backward logic, so please register the op in this way https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/fluid/operators/accuracy_op.cc#L97
There was a problem hiding this comment.
Done. You're definitely correct.
|
|
||
| Args: | ||
| x(Variable): Operand of *is_empty* | ||
| cond(Variable|None): Optional output variable to store the result of *is_empty* |
There was a problem hiding this comment.
Done. Thanks for pointing out!
| helper = LayerHelper("is_empty", **locals()) | ||
| if cond is None: | ||
| cond = helper.create_tmp_variable(dtype='bool') | ||
| cond.stop_gradient = True |
There was a problem hiding this comment.
Please check the data type in else part.
paddle/fluid/operators/is_empty_op.h
Outdated
| // get output | ||
| auto* output_tensor = context.Output<framework::LoDTensor>("Out"); | ||
|
|
||
| output_tensor->Resize({1}); |
There was a problem hiding this comment.
This is unnecessary, since the shape is already set in InferShape
| cond(Variable|None): Optional output variable to store the result | ||
| of *is_empty* | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
After this section, we can add Raises section. Please refer to https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/fluid/layers/nn.py#L149
Resolved #10512