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

【Hackathon 6th No. 7】Add sinc / sinc_ API to Paddle -part #63521

Merged
merged 28 commits into from
May 16, 2024

Conversation

NKNaN
Copy link
Contributor

@NKNaN NKNaN commented Apr 15, 2024

PR Category

User Experience

PR Types

New features

Description

Add sinc / sinc_ API to Paddle

Copy link

paddle-bot bot commented Apr 15, 2024

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added the contributor External developers label Apr 15, 2024
@NKNaN NKNaN changed the title 【Hackthon 6th No. 7】Add sinc / sinc_ API to Paddle 【Hackathon 6th No. 7】Add sinc / sinc_ API to Paddle Apr 15, 2024
Returns:
out (Tensor): The Tensor of elementwise computed normalized sinc result.
"""
if not isinstance(x, (paddle.Tensor, Variable, paddle.pir.Value)):
Copy link
Contributor

Choose a reason for hiding this comment

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

propose adding dtype judgment, also adding in test_sinc.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

if not isinstance(x, (paddle.Tensor, Variable, paddle.pir.Value)):
raise TypeError(f"x must be tensor type, but got {type(x)}")

if x.dtype not in [
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can use "check_variable_and_dtype", this will make your code look more concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised

@NKNaN
Copy link
Contributor Author

NKNaN commented Apr 22, 2024

The CI-Coverage check for python code included the last two line of signbit API accidentally.

@luotao1 luotao1 added the API label Apr 24, 2024
@@ -487,6 +487,8 @@
signbit,
sin,
sin_,
sinc,
Copy link
Contributor

Choose a reason for hiding this comment

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

also add it in the list of all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

res = paddle.sinc(x)
static_result = exe.run(feed={'x': x_data}, fetch_list=[res])[0]

def test_inplace(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

the type error of inplace API also should be test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@@ -803,7 +805,8 @@
'standard_gamma',
'sinh',
'sinh_',
'round',
'sinc',
'sinc_' 'round',
Copy link
Contributor

Choose a reason for hiding this comment

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

attention code style here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops..
fixed

Copy link
Contributor

@zxcd zxcd left a comment

Choose a reason for hiding this comment

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

LGTM

check_variable_and_dtype(
x,
"x",
[
Copy link
Contributor

Choose a reason for hiding this comment

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

should we support bfloat16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.
bfloat16 added

"sinc",
)

tmp = paddle.where(x != 0, x, paddle.full_like(x, 1.0e-20))
Copy link
Contributor

Choose a reason for hiding this comment

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

why use 1.0e-20, if data type of x is float16, will there be underflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.0e-20 is used in numpy.sinc, but there is underflow problem when data type is float16.
Fixed the problem using paddle.isnan before returning the final value.

Copy link
Contributor

@jeff41404 jeff41404 May 9, 2024

Choose a reason for hiding this comment

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

Can we support backward gradient calculation, especially when x=0?

Copy link
Contributor

Choose a reason for hiding this comment

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

shall we add test case for calculating 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.

shall we add test case for calculating gradient

I will add it

Copy link
Contributor Author

@NKNaN NKNaN May 11, 2024

Choose a reason for hiding this comment

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

Can we support backward gradient calculation, especially when x=0?

when x=0,the gradient should be 0:

$$( \frac{\sin(\pi x)}{\pi x} )^{\prime} = \pi \frac{\pi x \cos(\pi x) - \sin(\pi x)}{\pi^2 x^2}$$

by LHopital's rule,

$$ \lim_{x \rightarrow 0} \frac{\pi x \cos(\pi x) - \sin(\pi x)}{\pi^2 x^2} = \lim_{x \rightarrow 0} \frac{ [\pi \cos(\pi x) - \pi^2 x \sin(\pi x) ] - \pi \cos(\pi x)}{2 \pi^2 x} = \lim_{x \rightarrow 0} -\frac{\sin(\pi x)}{2} = 0$$

The torch example:

x = torch.rand(2,3)
x[0,0] = 0.0
x.requires_grad = True
s = torch.sinc(x).sum()
s.backward()
x.grad
# tensor([[ 0.0000, -1.2824, -1.0302],
#        [-1.3687, -1.0435, -1.1842]])

The current implementation using paddle.where can return gradient value 0 if x = 0

check_variable_and_dtype(
x,
"x",
[
Copy link
Contributor

Choose a reason for hiding this comment

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

should we support bfloat16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.
bfloat16 added

"sinc_",
)

paddle.where_(x != 0, x, paddle.full_like(x, 1.0e-20))
Copy link
Contributor

Choose a reason for hiding this comment

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

same issue as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the problem using paddle.isnan before returning the final value.

@PaddlePaddle PaddlePaddle locked and limited conversation to collaborators May 7, 2024
@PaddlePaddle PaddlePaddle unlocked this conversation May 7, 2024
@luotao1
Copy link
Contributor

luotao1 commented May 13, 2024

image @NKNaN 看下这里的覆盖率,如果本地单测有覆盖到,请截图贴上。

@NKNaN
Copy link
Contributor Author

NKNaN commented May 13, 2024

@NKNaN 看下这里的覆盖率,如果本地单测有覆盖到,请截图贴上。

这个是 isreal 的内容,不知道为啥这三行也会算到 coverage 检测里面

@luotao1
Copy link
Contributor

luotao1 commented May 14, 2024

这个是 isreal 的内容,不知道为啥这三行也会算到 coverage 检测里面

收到,已经豁免了

Copy link
Contributor

@jeff41404 jeff41404 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

@sunzhongkai588 sunzhongkai588 left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1 luotao1 changed the title 【Hackathon 6th No. 7】Add sinc / sinc_ API to Paddle 【Hackathon 6th No. 7】Add sinc / sinc_ API to Paddle -part May 15, 2024
@luotao1 luotao1 merged commit 5efe635 into PaddlePaddle:develop May 16, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants