Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add empty_like op (python, and unit test), use c++ implementation of … #27287

Merged
merged 1 commit into from
Sep 17, 2020

Conversation

windstamp
Copy link
Contributor

@windstamp windstamp commented Sep 14, 2020

PR types

New features

PR changes

APIs

Describe

add empty_like op.

Returns an uninitialized tensor with the same size as input.

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Sep 14, 2020

✅ This PR's description meets the template requirements!
Please wait for other CI results.

type='empty',
inputs={},
outputs={'Out': [out]},
attrs={'shape': x.shape,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x.shape在如果输入shape存在负数的时候是否在empty的代码考虑了?

Copy link
Contributor Author

@windstamp windstamp Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. 对于参数 shape,目前 empty op 的 C++ 实现有问题,没有校验里面的值是否为负数,不过 review 意见已经指出来了,会在这个 op 里面修复。

只是这里我有个疑问,假设 x 校验通过后,x.shape 是不是应该是没有问题的。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需要用过输入ShapeTensor来设置输出的shape。

Copy link
Contributor Author

@windstamp windstamp Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks。已经修改。通过 paddle.shape(x) 可以返回正确的 shape,这个 shape 是一个 tensor,然后作为 empty op 的 ShapeTensor 传入。

这里同时修改了一下 shape op,之前不支持 bool 类型,现在支持。同时添加了 shape op 的文档预览。

Copy link
Contributor

@wangchaochaohu wangchaochaohu Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

添加一个这种情况静态图的单测吧

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks。已添加对应单侧。

"is [%s]",
dims);
platform::errors::InvalidArgument(
"ShapeError: The shape of Tensor in list must be [1]. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

去掉ShapeError:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks。已修改。

@windstamp windstamp force-pushed the empty_like_op_3 branch 2 times, most recently from ab04e59 to bbb1bf9 Compare September 15, 2020 07:27
@windstamp
Copy link
Contributor Author

image

REGISTER_OP_CPU_KERNEL(shape, ops::ShapeKernel<int>, ops::ShapeKernel<int32_t>,
ops::ShapeKernel<int64_t>, ops::ShapeKernel<float>,
ops::ShapeKernel<double>);
REGISTER_OP_CPU_KERNEL(shape, ops::ShapeKernel<bool>, ops::ShapeKernel<int>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这shape_op咋同时注册了intint32_t类型的kernel啊。不过这个与你这个PR的功能无关了。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks。我把 int32_t 去除吧。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已去除 int32_t


class TestEmptyLikeAPI_Static(unittest.TestCase):
def __check_out__(self, out):
data_type = convert_dtype(out.dtype)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个函数的实现,看起来和上一个类里面的__check__out__实现有很多重复的代码

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks。我优化一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已优化


self.dst_dtype = dtype
self.dst_shape = x.shape
self.__check_out__(res[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

封装一下,避免引入这么多重复的代码

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks。我优化一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已优化

wangchaochaohu
wangchaochaohu previously approved these changes Sep 16, 2020
Copy link
Contributor

@wangchaochaohu wangchaochaohu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Xreki
Xreki previously approved these changes Sep 16, 2020
Copy link
Contributor

@Xreki Xreki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -11229,7 +11229,7 @@ def shape(input):
input.shape = [3, 2]

Args:
input (Variable): The input can be N-D Tensor or SelectedRows with data type float16, float32, float64, int32, int64.
input (Variable): The input can be N-D Tensor or SelectedRows with data type bool, float16, float32, float64, int32, int64.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

跟这个PR无关。
这个API现在直接以paddle.shape的形式暴露给用户了,相应的,文档和示例代码需要更新

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. 这个我要怎么改呢,因为我里面需要用到 paddle.shape(x) 这个 op 来返回 x 的 shape。这里,x 支持 bool 类型,而 paddle.shape(x) 并不支持 bool 类型,所以想在这个 PR 中直接修改掉。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

要不这个 PR 中先保留这些修改,涉及文档和示例的地方后面再提一个 PR?你看这样可以吗。

If the ``dtype`` is None, the data type of Tensor is same with ``x``.

Args:
x(Tensor): The input tensor which specifies shape and data type. The data type can be bool, float32, float64, int32, int64.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

说明x的时候不需要说明data type吧?

Copy link
Contributor Author

@windstamp windstamp Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks。我修改一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我看了一下 full_like,也是这样描述 x 的,要不这里维持原样,不修改?


Args:
x(Tensor): The input tensor which specifies shape and data type. The data type can be bool, float32, float64, int32, int64.
dtype(np.dtype|str, optional): Data type of the output Tensor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

说明一下default value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. 我修改一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改,参考 full_like 改成如下:

        dtype(np.dtype|str, optional): The data type of output. The data type can be one
            of bool, float16, float32, float64, int32, int64. The default value is None, which means the output 
            data type is the same as input.

paddle.set_device("cpu") # and use cpu device

x_data = np.random.random((2, 3)).astype("float32")
x = paddle.to_tensor(x_data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x可以直接用paddle.randn创建。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. 我修改一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改。

…empty op,

and optimize the c++ implmentation of empty op as PR#26659 reviews,
and add bool for shape op.
@windstamp
Copy link
Contributor Author

image

Copy link
Contributor

@jzhang533 jzhang533 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zhiqiu zhiqiu self-requested a review September 17, 2020 10:44
@Xreki Xreki merged commit 515efe4 into PaddlePaddle:develop Sep 17, 2020
@windstamp windstamp deleted the empty_like_op_3 branch April 8, 2021 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants