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

【PaddlePaddle Hackathon 2】8、为 Paddle 新增 nanmean API #40472

Merged
merged 49 commits into from
Apr 6, 2022

Conversation

Li-fAngyU
Copy link
Contributor

@Li-fAngyU Li-fAngyU commented Mar 11, 2022

PR types

New features

PR changes

APIs

Describe

ISSUE链接:#40327
RFC的PR链接:PaddlePaddle/community#48
中文文档PR链接:PaddlePaddle/docs#4294
为 Paddle 新增 nanmean API

paddle.nanmean 扩展了 paddle.mean API 的功能,如果输入Tensor中有nan值, paddle.mean在计算中会将涉及nan值的结果都置为nan,而 paddle.nanmean 会跳过nan值。比如输入数据 x = [[nan, 1. , 2. ], [3. , 4. , 5. ]],x.mean() 得到 [nan],x.mean(0) 得到 [nan, 2.5, 3.5],x.nanmean() 得到 [3.],x.nanmean(0) 得到 [3., 2.5, 3.5]。此API需支持的调用路径为:paddle.nanmean 和 Tensor.nanmean 。

@paddle-bot-old
Copy link

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

@Ligoml Ligoml added the contributor External developers label Mar 18, 2022
@paddle-bot-old
Copy link

PR格式检查通过,你的PR将接受Paddle专家以及开源社区的review,请及时关注PR动态。
The format inspection passed. Your PR will be reviewed by experts of Paddle and developers from the open-source community. Stay tuned.

@@ -967,6 +967,81 @@ def nansum(x, axis=None, dtype=None, keepdim=False, name=None):
return sum(tmp_tensor, axis, dtype, keepdim, name)


def nanmean(x,axis=None,keepdim=None,name=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

keepdim=False is better?keep same with other API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for your suggestion! keep same with other API is the better way. i will update this

axis = [axis]
if axis == None:
return paddle.mean(x[~paddle.isnan(x)], keepdim=keepdim,name=name)
check_variable_and_dtype(x, 'x/input',
Copy link
Contributor

Choose a reason for hiding this comment

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

just 'x' instead of 'x/input'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is refer to the code of paddle.mean.
'x/input' seems to be used only as a input name when raise a type/dtype error.

if axis == None:
return paddle.mean(x[~paddle.isnan(x)], keepdim=keepdim,name=name)
check_variable_and_dtype(x, 'x/input',
['uint16', 'float16', 'float32', 'float64'],
Copy link
Contributor

Choose a reason for hiding this comment

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

dtype should be the same to description of x above. eg: 'uint16' is not in "x (Tensor): The input Tensor with data type float32, float64."

Copy link
Contributor Author

@Li-fAngyU Li-fAngyU Mar 18, 2022

Choose a reason for hiding this comment

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

This is refer to the code of paddle.mean
Although it require the input tensor must with data type of float32/64, but it also check allow data type of 'uint16' and 'float16' code is here.
Because of the issus describe here(paddle.nanmean extends the functionality of the paddle.mean API), so i just follow the code of paddle.mean

Comment on lines 1034 to 1035
if axis == None:
return paddle.mean(x[~paddle.isnan(x)], keepdim=keepdim,name=name)
Copy link
Contributor

Choose a reason for hiding this comment

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

can the logic of code L1041~L1042 below cover this branch? we need to handle the condition 'axis == None' alone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for your suggestion!
the logic of code L1041~L1042 below can cover this branch.
we don't need handle this condition alone.
i will take this advise and update this problem.


def setUp(self):
self.x_shape = [2, 3, 4, 5]
self.x = np.random.uniform(-1, 1, self.x_shape).astype(np.float32)
Copy link
Contributor

Choose a reason for hiding this comment

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

self.x does not have 'nan', we should cover all the test case in rfcs
also should include check gradient

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 for your suggestion! i will cover all the test case in next update.
but i am confused in check gradient, because i can't find the example of check gradient in the paddle.mean test file test_mean_op.py .
I will appreciate it if you can give me a example.

@Ligoml
Copy link
Contributor

Ligoml commented Mar 18, 2022

hi~有些细节需要注意一下 @Li-fAngyU
1、PR的标题要和ISSUE标题内容一样;
2、Describe 里要加上 ISSUE 链接 & RFC链接 & 中文文档链接

@Li-fAngyU Li-fAngyU changed the title 【hackathon + no.8】 【PaddlePaddle Hackathon 2】8、为 Paddle 新增 nanmean API Mar 18, 2022
@Li-fAngyU
Copy link
Contributor Author

@Ligoml Done!

修改了nanmean的axis参数的文档描述。
Ligoml
Ligoml previously approved these changes Apr 2, 2022
Copy link
Contributor

@Ligoml Ligoml left a comment

Choose a reason for hiding this comment

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

LGTM for docs

@paddle-bot-old
Copy link

paddle-bot-old bot commented Apr 2, 2022

祝贺你,你的PR测试通过,后续将会纳入飞桨的发版计划中,感谢你对飞桨开发者社区的参与。
Congratulations! The test passed and your PR will be released in our coming stable version. Thank you for your contribution to the open-source project of PaddlePaddle.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Apr 2, 2022

你的PR有最新反馈,请及时修改。
There’s the latest feedback about your PR. Please check.

XiaoguangHu01
XiaoguangHu01 previously approved these changes Apr 2, 2022
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

Ligoml
Ligoml previously approved these changes Apr 3, 2022
update example code
update example code of nanmean
Copy link
Contributor

@Ligoml Ligoml left a comment

Choose a reason for hiding this comment

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

LGTM for docs

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants