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

refine the concat Op test=develop #25307

Merged
merged 24 commits into from
Jul 27, 2020
Merged

refine the concat Op test=develop #25307

merged 24 commits into from
Jul 27, 2020

Conversation

wangchaochaohu
Copy link
Contributor

@wangchaochaohu wangchaochaohu commented Jul 1, 2020

PR types

Function optimization

PR changes

Ops

Describe

old

paddle.tensor.concat(input, axis=0, name=None):

new

paddle.tensor.concat(x, axis=0, name=None)

concat

image

image

fluid.concat

image

image

full
image
image

eye
image
image

linspace
image
image

full_like
image
image

index_select
image

image

zeros:
image
image

python/paddle/tensor/manipulation.py Show resolved Hide resolved
python/paddle/tensor/manipulation.py Outdated Show resolved Hide resolved
python/paddle/tensor/manipulation.py Outdated Show resolved Hide resolved
python/paddle/tensor/manipulation.py Outdated Show resolved Hide resolved
@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 7, 2020

Thanks for your contribution!
Please add test = develop in your commit message to trigger CI to ensure your PR can be merged.
See Paddle CI Manual for details.

1 similar comment
@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 7, 2020

Thanks for your contribution!
Please add test = develop in your commit message to trigger CI to ensure your PR can be merged.
See Paddle CI Manual for details.

GaoWei8
GaoWei8 previously approved these changes Jul 17, 2020
zhupengyang
zhupengyang previously approved these changes Jul 17, 2020
@wangchaochaohu wangchaochaohu dismissed stale reviews from zhupengyang and GaoWei8 via 48d00d5 July 17, 2020 09:04
zhupengyang
zhupengyang previously approved these changes Jul 17, 2020
GaoWei8
GaoWei8 previously approved these changes Jul 17, 2020
python/paddle/fluid/layers/tensor.py Show resolved Hide resolved
x_2 = fluid.data(shape=[2, 1, 4, 5], dtype='int32', name='x_2')
x_3 = fluid.data(shape=[2, 2, 4, 5], dtype='int32', name='x_3')
positive_1_int32 = paddle.fill_constant([1], "int32", 1)
positive_1_int64 = paddle.fill_constant([1], "int64", 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

为什么没有负数的case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已添加

:alias: paddle.concat,paddle.tensor.concat,paddle.tensor.manipulation.concat
:old_api: paddle.fluid.layers.concat

**Concat**
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.

Done

python/paddle/tensor/manipulation.py Outdated Show resolved Hide resolved
x(list): List of input Tensors with data type float32, float64, int32, int64.
axis(int|Variable, optional): A scalar with type ``int32`` or a ``Tensor``
with shape [1] and type ``int32``. Axis to compute indices along. The effective range
is [-R, R), where R is Rank(x). when axis<0, it works the same way
Copy link
Contributor

Choose a reason for hiding this comment

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

axis<0

这个地方不知道展示效果如何,是否要在运算符两侧加上空格

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

python/paddle/tensor/manipulation.py Outdated Show resolved Hide resolved
python/paddle/tensor/manipulation.py Show resolved Hide resolved
python/paddle/tensor/manipulation.py Show resolved Hide resolved
@wangchaochaohu wangchaochaohu dismissed stale reviews from GaoWei8 and zhupengyang via 1fbeb42 July 21, 2020 15:25
@PaddlePaddle PaddlePaddle deleted a comment from CLAassistant Jul 21, 2020
@CLAassistant
Copy link

CLAassistant commented Jul 21, 2020

CLA assistant check
All committers have signed the CLA.

python/paddle/fluid/layers/tensor.py Outdated Show resolved Hide resolved
out_1 = paddle.concat(x=[x_2, x_3], axis=1)
out_2 = paddle.concat(x=[x_2, x_3], axis=positive_1_int32)
out_3 = paddle.concat(x=[x_2, x_3], axis=positive_1_int64)
out_4 = paddle.concat(x=[x_2, x_3], axis=positive_1_int64)
Copy link
Contributor

Choose a reason for hiding this comment

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

copy&paste?忘了改成负数的case了?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已改

paddle/fluid/operators/concat_op.cc Outdated Show resolved Hide resolved
"""
:alias_main: paddle.concat
:alias: paddle.concat,paddle.tensor.concat,paddle.tensor.manipulation.concat
:old_api: paddle.fluid.layers.concat
Copy link
Contributor

Choose a reason for hiding this comment

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

@TCChenlong 确认一下,这里到底是怎么规定的,我看到有写update_api的,还有写old_api的

Copy link
Contributor

Choose a reason for hiding this comment

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

蓝翔老师,2.0里API的文档不需要写update_api或old_api,在版本发布时,会在2.0版本里的兼容1.8版本的API统一增加一行Note or Warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已删除

python/paddle/tensor/manipulation.py Show resolved Hide resolved
python/paddle/tensor/manipulation.py Outdated Show resolved Hide resolved
python/paddle/fluid/layers/tensor.py Outdated Show resolved Hide resolved
python/paddle/fluid/layers/tensor.py Show resolved Hide resolved
python/paddle/fluid/layers/tensor.py Outdated Show resolved Hide resolved
python/paddle/fluid/layers/tensor.py Outdated Show resolved Hide resolved
@@ -270,7 +270,7 @@ def concat(input, axis=0, name=None):

Args:
input(list): List of input Tensors with data type float16, float32, float64, int32,
int64. The data type of ``input`` must be same.
int64. All tensor in ``input`` must have same data type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int64. All tensor in ``input`` must have same data type.
int64. All Tensors in ``input`` must have the same data type.

python/paddle/fluid/layers/tensor.py Outdated Show resolved Hide resolved
@@ -332,6 +333,8 @@ def concat(input, axis=0, name=None):
check_variable_and_dtype(
x, 'input[' + str(id) + ']',
['float16', 'float32', 'float64', 'int32', 'int64'], 'concat')
if convert_dtype(x.dtype) != convert_dtype(input[0].dtype):
raise TypeError("All Tensor in the input must has same data type.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise TypeError("All Tensor in the input must has same data type.")
raise TypeError("All Tensors in the input must have same data type.")

同样一句话出现在几个位置,每次都不相同,请仔细检查一下是否还有其他问题

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

python/paddle/tensor/manipulation.py Outdated Show resolved Hide resolved
@@ -69,6 +71,7 @@ def concat(x, axis=0, name=None):
Raises:
TypeError: The dtype of `x` must be one of float16, float32, float64, int32 and int64.
TypeError: The `axis` must be int or Variable.
TypeError: All the tensor in ``x`` must have the same data type.
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.

Done

as axis+R. Default is 0.
name (str, optional): The default value is None. Normally there is no
need for user to set this property. For more information, please
refer to :ref:`api_guide_Name`.
Raises:
TypeError: The dtype of input must be one of float16, float32, float64, int32 and int64.
TypeError: The `axis` must be int or Variable.
TypeError: All the tensor in ``input`` must have the same data type.
TypeError: The ``axis`` must be int or Variable. The dtype of ``axis`` must be int32 or int64 when it's a Tensor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable要统一改为Tensor吧?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

后面关于2.0共性修改会做统一修改

Copy link
Contributor

@lanxianghit lanxianghit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@swtkiwi swtkiwi left a comment

Choose a reason for hiding this comment

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

LGTM,input应该变为x,Variable变为Tensor,这些私下沟通后面会单独提PR改

@wangchaochaohu wangchaochaohu merged commit 1e4ab72 into PaddlePaddle:develop Jul 27, 2020
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

7 participants