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

[DimExpr] Add ValueShapeExpr unittest #60056

Merged
merged 26 commits into from
Dec 21, 2023

Conversation

jiahy0825
Copy link
Contributor

PR types

Others

PR changes

Others

Description

pcard-76996

[DimExpr] Add ValueShapeExpr unittest

Copy link

paddle-bot bot commented Dec 15, 2023

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

// x => {shape: [S0, 2], value: nullopt}
ValueShapeDimExprs x_value_shape{x_shapes};
value2shape.emplace(x, x_value_shape);
// y => {shape: [1, S1, 2], value: nullopt}
Copy link
Contributor

Choose a reason for hiding this comment

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

感觉咱们的命名有点奇怪,数据结构是"ValueShape"DimExpr,但是实际数据存储,是{SHAPE: [1, S1, 2], VALUE: nullopt}

Copy link
Contributor Author

@jiahy0825 jiahy0825 Dec 18, 2023

Choose a reason for hiding this comment

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

收到,Thanks~考虑到命名改成 ShapeValue 可能会和 pir 中的 Value 命名混淆,这里我后续把数据存储的顺序更改下,改成先 Value 再 Shape 的顺序。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

本 PR 已修改

ValueShapeDimExprs x_value_shape{x_shapes};
value2shape.emplace(x, x_value_shape);
// y => {shape: [1, S1, 2], value: nullopt}
ValueShapeDimExprs y_value_shape{y_shapes};
Copy link
Contributor

Choose a reason for hiding this comment

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

想起另一个关于名字的问题,"Dim"Expr,但是实际定义应该是“符号表达式”,是不是叫SymExpr更合适一些

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~后续提个 PR 统一修改下

// y => {shape: [1, S1, 2], value: nullopt}
ValueShapeDimExprs y_value_shape{y_shapes};
value2shape.emplace(y, y_value_shape);
// extend_x => {shape: [2], value: [S0, 2]}
Copy link
Contributor

Choose a reason for hiding this comment

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

我记得ValueShape结构的定义里,两个数据都是optional的,但是如果前面的数据在任何情况下都有值,是不是没必要用optional了

Copy link
Contributor

Choose a reason for hiding this comment

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

我目前走了一下reshape的推导流程,感觉在需要使用第二个数据的场景下,第一个数据貌似已经没有意义了,未必需要填值,当然,目前看到的场景不多,不确定是否所有场景都是只需要两个数据中的一个

Copy link
Contributor Author

Choose a reason for hiding this comment

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

考虑到下面这种复杂场景,shape 的数据可能是需要的

def foo(x):
    x_extend_tensor = x.shape
    shape_array = x_extend_tensor.shape
    y  = fill_constant(1, shape_array_x)

可能还是要先保持 MakeConsistentValue 这种同时构造出 shapevalue 的函数,后续确认没有需求的时候,再做修改

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我记得ValueShape结构的定义里,两个数据都是optional的,但是如果前面的数据在任何情况下都有值,是不是没必要用optional了

本 PR 已修改

lanxianghit
lanxianghit previously approved these changes Dec 18, 2023
TEST(DimExpr, dim_expr_naive) {
DimExpr sym0 = DimExpr("S0");
DimExpr sym1 = DimExpr("S1");
DimExpr constant1 = DimExpr(1);
Copy link
Contributor

@lanxianghit lanxianghit Dec 18, 2023

Choose a reason for hiding this comment

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

DimExpr以及valueshape最好能加上Print接口,这样开发调试的时候打印信息会方便一些

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,后续提个 PR 出来

@@ -16,7 +16,14 @@ paddle_test(
pir
gtest)

paddle_test(symbol_dim_expr_test SRCS symbol_dim_expr_test.cc DEPS pir gtest)
cc_test_old(
Copy link
Contributor Author

@jiahy0825 jiahy0825 Dec 18, 2023

Choose a reason for hiding this comment

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

此处使用 cc_testpaddle_test 在 Windows-CI 下会出现报错:
error LNK2005: "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl paddle::framework::DataTypeToString(enum paddle::framework::proto::VarType_Type)"

@jiahy0825 jiahy0825 closed this Dec 21, 2023
@jiahy0825 jiahy0825 reopened this Dec 21, 2023
std::vector<DimExpr> x_shapes{DimExpr("S0"), DimExpr(2)};
std::vector<DimExpr> y_shapes{DimExpr(1), DimExpr("S1"), DimExpr(2)};
// x => {shape: [S0, 2], data: nullopt}
ShapeOrDataDimExprs x_value_shape{x_shapes};
Copy link
Contributor

Choose a reason for hiding this comment

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

x_value_shape
单测里类似这种地方的变量名也都改一下吧

Copy link
Contributor

Choose a reason for hiding this comment

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

可以下个PR再改,这个PR先合入

Copy link
Contributor Author

Choose a reason for hiding this comment

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

收到,下个 PR 改

@jiahy0825 jiahy0825 merged commit 291f523 into PaddlePaddle:develop Dec 21, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants