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 5th No.27】为 Paddle 新增 select_scatter API -part #59343

Merged
merged 13 commits into from
Dec 7, 2023

Conversation

YibinLiu666
Copy link
Contributor

@YibinLiu666 YibinLiu666 commented Nov 24, 2023

PR types

Others

PR changes

APIs

Description

#57262
新增select_scatter API
RFC:PaddlePaddle/community#757

Copy link

paddle-bot bot commented Nov 24, 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.

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.

从后续适配组合算子、AI编译器、接入新硬件等角度考虑,目前尽量是非必要不新增算子。由于这个算子的功能实际上是set_value的子集,不建议再新增算子,这个我们可以在RFC中进一步讨论,统一意见以后再实际写实现代码。

当然,从这个PR的kernel逻辑实现来看,预估会比set_value的kernel逻辑更高效。如果有兴趣的话,可以基于这个PR中kernel的实现逻辑,直接在set_value kernel中修改及优化,不过这已经超出了当前任务的范畴。

@YibinLiu666
Copy link
Contributor Author

@zoooo0820 已经按RFC实现了该接口,麻烦您有空的时候review一下。

@PaddlePaddle PaddlePaddle locked and limited conversation to collaborators Nov 30, 2023
@PaddlePaddle PaddlePaddle unlocked this conversation Nov 30, 2023
@YibinLiu666
Copy link
Contributor Author

@zoooo0820 CI都过了,麻烦您review一下

)

# map var to the new output
paddle.jit.api.ProgramTranslator.get_instance()._inplace_map.add(
Copy link
Contributor

Choose a reason for hiding this comment

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

这部分是动转静用来适配python__setitem__内置返回值为None的,这里API可以正常返回output,所以这句不需要了

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

res = paddle.select_scatter(x_tensor, value_tensor, 1, 1)
except Exception as error:
self.assertIsInstance(error, RuntimeError)

Copy link
Contributor

Choose a reason for hiding this comment

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

这里查错辛苦直接用 self.assertRaises吧,否则后续如果某些修改导致try中语句能过了,这个单测会检测不到。

此外,这两个case能拆成两个函数吗,命名上再清晰一些说明是检查的什么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.

Done

dtype=x.dtype
)
else:
output = helper.create_variable_for_type_inference(dtype=x.dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

这部分也是动转静用来适配python__setitem__内置返回值为None的,参考其他API写法,用else的分支即可

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

@YibinLiu666
Copy link
Contributor Author

@zoooo0820 CI已过,辛苦您再review一下

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

Embeds the values of the values tensor into x at the given index of axis.

Args:
x (Tensor) : The Destination Tensor.
Copy link
Contributor

Choose a reason for hiding this comment

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

should decribe x support what kinds of data types

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

@YibinLiu666
Copy link
Contributor Author

@jeff41404 修改doc之后LLM的CI不知道为什么一直过不了,coverage显示是我没修改的部分没覆盖到

@luotao1
Copy link
Contributor

luotao1 commented Dec 6, 2023

  • LLM是非required CI,可以不用考虑
  • Coverage确实是本PR没修改的代码没覆盖到,可以最后豁免

Embeds the values of the values tensor into x at the given index of axis.

Args:
x (Tensor) : The Destination Tensor. Supported data types are `bool`, `float16`, `float32`, `float64`, `uint8`, `int8`, `int16`, `int32`, `int64`, `bfloat16`, `complex64`, `complex64`.
Copy link
Contributor

Choose a reason for hiding this comment

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

complex64, complex64 should be complex64, complex128 ?

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


Args:
x (Tensor) : The Destination Tensor. Supported data types are `bool`, `float16`, `float32`, `float64`, `uint8`, `int8`, `int16`, `int32`, `int64`, `bfloat16`, `complex64`, `complex64`.
values (Tensor) : The tensor to embed into x. Supported data types are `bool`, `float16`, `float32`, `float64`, `uint8`, `int8`, `int16`, `int32`, `int64`, `bfloat16`, `complex64`, `complex64`.
Copy link
Contributor

Choose a reason for hiding this comment

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

complex64, complex64 should be complex64, complex128 ?

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

@luotao1
Copy link
Contributor

luotao1 commented Dec 6, 2023

可以提交对应的中文文档

Copy link
Contributor

@sunzhongkai588 sunzhongkai588 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 changed the title 【Hackathon 5th No.27】为 Paddle 新增 select_scatter API 【Hackathon 5th No.27】为 Paddle 新增 select_scatter API -part Dec 6, 2023
@luotao1 luotao1 merged commit 3a0aeaa into PaddlePaddle:develop Dec 7, 2023
28 of 29 checks passed
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.

None yet

5 participants