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】9、为 Paddle 新增 logspace API #41261

Merged
merged 21 commits into from
Apr 20, 2022

Conversation

BrilliantYuKaimin
Copy link
Contributor

@BrilliantYuKaimin BrilliantYuKaimin commented Apr 1, 2022

PR types

New features

PR changes

APIs

Describe

增加了paddle.logspace。具体地,paddle.logspace(start, stop, num, base)能产生一个以base**start为首项、以base**end为末项、共有num项的等比数列。
Issue:#40326
设计文档:PaddlePaddle/community#56
中文文档:PaddlePaddle/docs#4497

@paddle-bot-old
Copy link

paddle-bot-old bot commented Apr 1, 2022

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.

@Ligoml
Copy link
Contributor

Ligoml commented Apr 2, 2022

CI挂了很多,需要调整代码,待CI大部分通过后开启review

Copy link
Contributor

@betterpig betterpig left a comment

Choose a reason for hiding this comment

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

代码规范,单测case考虑周全。几处细节改一下即可。

framework::OpKernelType GetKernelTypeForVar(
const std::string &var_name, const framework::Tensor &tensor,
const framework::OpKernelType &expected_kernel_type) const override {
return expected_kernel_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

返回的OpKernelType应该是由expected_kernel_type.data_type_和tensor的place、layout所构造的,不能直接返回expected_kernel_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.

完成

limitations under the License. */

#include <string>
#include "paddle/fluid/framework/infershape_utils.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

C++基础头文件和项目自身的头文件之间空一行,方便区分。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

// limitations under the License.

#include <cmath>
#include "paddle/phi/kernels/logspace_kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

C++基础头文件和项目自身的头文件之间空一行,方便区分。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成



if __name__ == "__main__":
unittest.main()
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.

完成

out_data[i] = static_cast<T>(std::pow(
base_data, start_data + step * i));
} else {
out_data[i] = static_cast<T>(std::pow(
Copy link
Contributor

Choose a reason for hiding this comment

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

这里注明一下:当pow的结果非整数,dytpe为int时,会采用去尾法,对齐numpy。

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/logspace_op.cc中增加了相关说明,这样可以吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

可以。

betterpig
betterpig previously approved these changes Apr 11, 2022
Copy link
Contributor

@betterpig betterpig left a comment

Choose a reason for hiding this comment

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

LGTM

def logspace(start, stop, num, base=10.0, dtype=None, name=None):
r"""
This OP return fixed number of logarithmical-evenly spaced values within a given interval.
Args:
Copy link
Contributor

Choose a reason for hiding this comment

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

此处用Parameters 较好

Copy link
Contributor

Choose a reason for hiding this comment

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

args也可以,不需要重新跑CI。

Copy link
Contributor

Choose a reason for hiding this comment

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

英文模板中是args,这边应该不需要改。但是英文文档预览报错,这里需要检查一下~
image

dingjiaweiww
dingjiaweiww previously approved these changes Apr 11, 2022
@betterpig
Copy link
Contributor

需要添加对应的中文文档,提交PR到PaddlePaddle/docs仓库。并参考文档预览方法,生成该API的中英文文档预览链接,以供检查。

@BrilliantYuKaimin
Copy link
Contributor Author

需要添加对应的中文文档,提交PR到PaddlePaddle/docs仓库。并参考文档预览方法,生成该API的中英文文档预览链接,以供检查。

已经在此PR的描述中提到中文文档的PR。

@paddle-bot-old
Copy link

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

@@ -1583,6 +1584,121 @@ def linspace(start, stop, num, dtype=None, name=None):
return out


def logspace(start, stop, num, base=10.0, dtype=None, name=None):
r"""
This OP return fixed number of logarithmical-evenly spaced values within a given interval.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 与中文文档保持一致,去掉 this OP的描述,尽量在第一句话里描述清楚这个API的作用
  • 若中文文档要加note,则英文也需要增加

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

def logspace(start, stop, num, base=10.0, dtype=None, name=None):
r"""
This OP return fixed number of logarithmical-evenly spaced values within a given interval.
Args:
Copy link
Contributor

Choose a reason for hiding this comment

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

英文模板中是args,这边应该不需要改。但是英文文档预览报错,这里需要检查一下~
image

Tensor: the output data type will be float32, float64. The 1-D tensor with
fixed number of evenly spaced values, the data shape of this tensor
is :math:`[num]` . If the :attr:`num` is set 1, the output tensor just
has the value with exponential of :attr:`start` with base :attr:`base`.
Copy link
Contributor

Choose a reason for hiding this comment

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

image

这里显示乱掉了,建议在关键字段前后增加空行,保证官网文档的正确解析

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

Ligoml
Ligoml previously approved these changes Apr 15, 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

DDDivano
DDDivano previously approved these changes Apr 15, 2022
@@ -0,0 +1,80 @@
/* Copyright (c) 2022 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

@chenwhql chenwhql Apr 15, 2022

Choose a reason for hiding this comment

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

license的格式有点问题,缺失空行和缩进,可以参考其他文件修改一下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

ctx.GetPlace());
}

framework::OpKernelType GetKernelTypeForVar(
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.

完成

#include "paddle/phi/backends/cpu/cpu_context.h"
#include "paddle/phi/core/kernel_registry.h"
#include "paddle/phi/kernels/funcs/data_type_transform.h"
#include "paddle/phi/kernels/logspace_kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

头文件的include顺序需要调整下,可以参考说明:https://zh-google-styleguide.readthedocs.io/en/latest/google-cpp-styleguide/headers/#include

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

// See the License for the specific language governing permissions and
// limitations under the License.

#include "paddle/fluid/platform/device/gpu/gpu_primitives.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

同上哈,麻烦也调整下,#include "paddle/phi/kernels/logspace_kernel.h"在最前面,用空行隔开

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

@@ -1614,6 +1615,130 @@ def linspace(start, stop, num, dtype=None, name=None):
return out


def logspace(start, stop, num, base=10.0, dtype=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.

为什么新增API是在fluid下面的,fluid后面是要移除的,这个是不是加到python/paddle/tensor目录下比较好?

Copy link
Contributor Author

@BrilliantYuKaimin BrilliantYuKaimin Apr 15, 2022

Choose a reason for hiding this comment

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

任务的issue #40326 说放在这里。我看linspace已经被移到了python/paddle/tensor/creation.py,那就把logspace也放到这里吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

是的,任务issue描述有误,已更新

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完成

// See the License for the specific language governing permissions and
// limitations under the License.

#include "paddle/fluid/platform/device/gpu/gpu_primitives.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

这里gpu_primitives.h看起来好像没有使用?是否可以移除?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已移除

@BrilliantYuKaimin BrilliantYuKaimin dismissed stale reviews from DDDivano and Ligoml via 3530acf April 16, 2022 10:28
Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,27 @@
/* Copyright (c) 2022 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的license格式好像还是有点问题,可以后续再完善下

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

@jeff41404 jeff41404 merged commit a3c50c4 into PaddlePaddle:develop Apr 20, 2022
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.

8 participants