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

Fix inconsistency between index_fill and index_fill_ #59863

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

Patrick-Star125
Copy link
Contributor

@Patrick-Star125 Patrick-Star125 commented Dec 9, 2023

PR types

Bug fixes

PR changes

APIs

Description

我之前错误的认为transpose是inplace的导致两个API最终结果不相等,这里将transpose去除,直接对x赋值

会增加一些的赋值成本,但是transpose没有inplace版本,目前paddle也没有可以替代实现交换轴的inplace API

Link

相关PR:#57416

Copy link

paddle-bot bot commented Dec 9, 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.

@paddle-bot paddle-bot bot added the contributor External developers label Dec 9, 2023
Copy link

paddle-bot bot commented Dec 9, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@@ -1621,7 +1621,7 @@ class TestDygraphInplaceIndexFill(TestDygraphInplace):
def init_data(self):
self.input_var_numpy = np.random.random((20, 40))
self.dtype = "float32"
self.axis = 0
self.axis = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

这个修改是否是必要的呢,目前axis=0是否表现正常?

Copy link
Contributor Author

@Patrick-Star125 Patrick-Star125 Dec 11, 2023

Choose a reason for hiding this comment

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

axis=0是正常的,axis=1覆盖情况更广,之前就是因为axis=0导致没有测试到这个API的inplace版本有不一致问题

if inplace:
paddle.transpose(x, perm)
Copy link
Contributor

Choose a reason for hiding this comment

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

目前有inplace版本paddle.transpose_

此外,不建议在该API中使用索引语法实现赋值,会带来解析索引的耗时,此前使用transpose + index_put的逻辑,本质上是已经根据该场景解析过后决定了要调用哪些API

Copy link
Contributor Author

@Patrick-Star125 Patrick-Star125 Dec 11, 2023

Choose a reason for hiding this comment

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

1.目前paddle文档上没有paddle.transpose_
2.使用paddle.transpose_实现会有inplace_version跳变问题,inplace_version貌似在python端只能增不能减,所以我更改了一部分version相关测试

self.assertEqual(var.inplace_version, 7)

def test_backward_error(self):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

这个error的case跳过是什么原因呢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

一样是inplace_version的问题,已添加

@co63oc
Copy link
Contributor

co63oc commented Dec 15, 2023

@zoooo0820 麻烦确认下是否可以合入,https://github.com/PaddlePaddle/PaConvert/ 中需要增加这个API的单测使用

@co63oc
Copy link
Contributor

co63oc commented Dec 15, 2023

@Patrick-Star125
image
PR types 需要是 Bug fixes

Copy link
Contributor

@zoooo0820 zoooo0820 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 merged commit 6a39f0b into PaddlePaddle:develop Dec 15, 2023
@luotao1
Copy link
Contributor

luotao1 commented Dec 15, 2023

@co63oc 该PR已合入

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants