Skip to content

Gs loss branch #50761

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 0 commits into from
Closed

Gs loss branch #50761

wants to merge 0 commits into from

Conversation

@Atlantisming
Copy link
Contributor

@Atlantisming Atlantisming commented Feb 22, 2023 •
edited
Loading

PR types

New features

PR changes

APIs

Describe

rfc 文档链接：PaddlePaddle/community#372
中文文档链接：PaddlePaddle/docs#5623

This was referenced Feb 22, 2023
Closed
Merged
Merged
@paddle-bot paddle-bot bot added the contributor External developers label Feb 22, 2023
@GGBond8488
Copy link
Contributor

https://www.paddlepaddle.org.cn/documentation/docs/zh/dev_guides/code_contributing_path_cn.html 按照官网贡献指南，安装pre-commit

GGBond8488
GGBond8488 reviewed Feb 23, 2023
View reviewed changes
python/paddle/tensor/math.py Outdated
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.

是之前尝试文档修复时拉取的代码，重新拉取后仍存在，是否直接删除就行了。

Copy link
Contributor

Choose a reason for hiding this comment

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

直接删除吧

python/paddle/nn/layer/loss.py Outdated
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.

Traceback (most recent call last): File "D:/PyWorkspace/Paddle/python/paddle/fluid/tests/unittests/test_gaussian_nll_loss.py", line 142, in test_api self.test_static_case(type='float64') File "D:/PyWorkspace/Paddle/python/paddle/fluid/tests/unittests/test_gaussian_nll_loss.py", line 125, in test_static_case res = exe.run( File "D:\Anaconda\envs\paddle_devcpu\lib\site-packages\paddle\fluid\executor.py", line 1586, in run res = self._run_impl( File "D:\Anaconda\envs\paddle_devcpu\lib\site-packages\paddle\fluid\executor.py", line 1830, in _run_impl self._feed_data(program, feed, feed_var_name, scope) File "D:\Anaconda\envs\paddle_devcpu\lib\site-packages\paddle\fluid\executor.py", line 1092, in _feed_data check_feed_shape_type(var, cur_feed) File "D:\Anaconda\envs\paddle_devcpu\lib\site-packages\paddle\fluid\executor.py", line 256, in check_feed_shape_type raise ValueError( ValueError: The data type of fed Variable 'Var' must be 'float32', but received 'float64'

添加了 数据类型测试，在静态图运行抛出以上数值错误，查询调用的API文档均可以支持float计算，请问是不是静态图测试有些问题？

Copy link
Contributor

Choose a reason for hiding this comment

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

前面padle.static.data 创建data的时候显示指定dtype，和你真实的输入保持一致

python/paddle/nn/functional/loss.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

注释改为paddle的术语:size->shape

python/paddle/fluid/tests/unittests/test_gaussian_nll_loss.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

测试需要覆盖所有的情况，# e.g. input.size = (10, 2, 3), var.size = (10, 2) 类似你代码中提到的情况

python/paddle/fluid/tests/unittests/test_gaussian_nll_loss.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

最好显示指定rtol以及atol

python/paddle/nn/layer/loss.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

这里提示应该改成GaussianNLLLoss

GGBond8488
GGBond8488 reviewed Feb 23, 2023
View reviewed changes
Copy link
Contributor

@GGBond8488 GGBond8488 left a comment

Choose a reason for hiding this comment

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

可能我的意思没表达清楚，
不是删除整个文件，是将你的math.py更改退回

@Atlantisming
Copy link
Contributor Author

可能我的意思没表达清楚， 不是删除整个文件，是将你的math.py更改退回

抱歉，我也以为删除的是change，正在解决conflict回滚

@paddle-bot
Copy link

paddle-bot bot commented Feb 23, 2023

很抱歉，经过我们的反复讨论，你的PR暂未达到合入标准，请阅读飞桨原生算子开发规范，你可以重新提交新的PR，我们先将此PR关闭，感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle Custom Operator Design Doc). You can also submit an new one. Thank you.

@Atlantisming Atlantisming deleted the GsLoss_branch branch February 23, 2023 11:57
@Ligoml Ligoml mentioned this pull request Mar 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@GGBond8488 GGBond8488 GGBond8488 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@luotao1 luotao1

@GGBond8488 GGBond8488

@cloud2009 cloud2009

Labels

contributor External developers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Atlantisming @GGBond8488 @luotao1 @cloud2009