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 No.5] tril_indices OP #41639

Merged
merged 49 commits into from
May 20, 2022
Merged

Conversation

xiaoguoguo626807
Copy link
Contributor

@xiaoguoguo626807 xiaoguoguo626807 commented Apr 11, 2022

PR types

New features

PR changes

OPs

Describe

add tril_indices CPU kernel python API and unit test
fix issue:#40330
RFC https://github.com/PaddlePaddle/community/blob/master/rfcs/APIs/20220323_api_design_for_tril_indices.md
中文文档PR PaddlePaddle/docs#4599

@paddle-bot-old paddle-bot-old bot added contributor External developers status: proposed labels Apr 11, 2022
@paddle-bot-old
Copy link

paddle-bot-old bot commented Apr 11, 2022

✅ This PR's description meets the template requirements!
Please wait for other CI results.

@paddle-bot-old
Copy link

你的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.

@CLAassistant
Copy link

CLAassistant commented Apr 11, 2022

CLA assistant check
All committers have signed the CLA.

modify english document

modify english document

modify document

modify document
void TrilIndicesInferMeta(
int rows, int cols, int offset, DataType dtype, MetaTensor* out) {
// number of elements in the first row of the tril,bounded by [0, cols]
auto m_first_row =
Copy link
Contributor

@jeff41404 jeff41404 May 9, 2022

Choose a reason for hiding this comment

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

use n_first_row is better? the form of variable name is unified with n_row_all and n_row_trapezoid below

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

auto m_first_row =
offset > 0 ? std::min<int64_t>(cols, 1 + offset) : rows + offset > 0;
// number of elements in the last row of the tril, bounded by [0, cols]
auto m_last_row =
Copy link
Contributor

Choose a reason for hiding this comment

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

use n_last_row is better? the form of variable name is unified with n_row_all and n_row_trapezoid below

Ligoml
Ligoml previously approved these changes May 10, 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

jeff41404
jeff41404 previously approved these changes May 10, 2022
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

@@ -1739,3 +1739,92 @@ def complex(real, imag, name=None):
attrs = {}
helper.append_op(type=op_type, inputs=inputs, attrs=attrs, outputs=outputs)
return out


def tril_indices(rows, cols, offset=0, dtype='int64'):
Copy link
Contributor

Choose a reason for hiding this comment

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

这里参数名用单数稍好一些,rows -> row, cols -> col
复数一般用来表示列表,比如,slice这个api中,axes, starts, ends都是用来接受列表的输入
https://www.paddlepaddle.org.cn/documentation/docs/zh/api/paddle/slice_cn.html#slice

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

@xiaoguoguo626807 xiaoguoguo626807 dismissed stale reviews from jeff41404 and Ligoml via 1670abc May 10, 2022 09:55
Ligoml
Ligoml previously approved these changes May 11, 2022
expected_result = np.tril_indices(4, 2)
self.assertEqual((out.numpy() == expected_result).all(), True)

def test_default_CPU(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

测默认值就不分CPU和GPU吧,节省一点case时间,目前有单测超时的可能(15s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已改,但是对测试总时间影响不大

with paddle.static.program_guard(paddle.static.Program(),
paddle.static.Program()):
data1 = paddle.tril_indices(4, 4, -1)
exe1 = paddle.static.Executor(place)
Copy link
Contributor

Choose a reason for hiding this comment

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

每个case测一个值就可以,或者静态图 为 正offset,动态图为 负offset,节省一些单测时间

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.

看下超时能不能过吧,如果还超时,那就需要再继续减少case了,目前不允许单测超过15s

Copy link
Contributor

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

@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 75db5b8 into PaddlePaddle:develop May 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.

7 participants