-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 5 No.2】Add index_fill / index_fill_ API to Paddle -part #57416
Conversation
Sorry to inform you that c49cd3d's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
Sorry to inform you that dfb2947's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static-check中提示示例代码格式问题,辛苦也再根据报错信息中给出的参考文档修改下
|
||
if inplace: | ||
x[:] = out | ||
return x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
此处,如果是inplace,是否可以不要调用clone+setitem赋值,而是直接使用index_put_赋值; 如果非inplace,是否可以不需要额外的clone操作
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the implementation solution in rfc shoule be also changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current implementation is same with rfc API design already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in rfc this line: out = paddle.clone(x)
at else branch need to be delete ?
self.x_shape = (10, 15, 10) | ||
self.axis = 1 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
补充下complex类型的测试吧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index_put不支持complex类型的输入
增加了float16类型的测试
test/legacy_test/test_index_fill.py
Outdated
ref_res = compute_index_put_ref( | ||
self.x_np, self.axis, self.index_np, self.value | ||
) | ||
np.testing.assert_allclose(ref_res, pd_res, atol=1e-5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这几个assert能否使用默认的atol阈值
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Done |
@Patrick-Star125 辛苦再看下static-check中显示的示例代码格式,以及单测中atol阈值设置这两个问题呢 |
@zoooo0820 似乎static-check中报错不太明确,我主要参考这个PR的写法,样例代码在我线下都是可以正常执行的 |
详细规范请务必参考 python文档示例代码规范 |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if axis < 0: | ||
axis = axis + x_dim | ||
|
||
if not (isinstance(axis, int)) or (axis > x_dim - 1) or axis < -x_dim: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The negative axis
has been processed in L5183 above, so the judgment condition here should be axis < 0
not axis < -x_dim
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
python/paddle/tensor/manipulation.py
Outdated
axis (int): The dimension along which to index. | ||
index (Tensor): The 1-D Tensor containing the indices to index. | ||
The data type of ``index`` must be int32 or int64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of parameters in the document should be consistent with the order of parameters in the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
test/legacy_test/test_index_fill.py
Outdated
paddle.enable_static() | ||
|
||
|
||
def compute_index_put_ref(x, axis, index, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compute_index_fill_ref
is more suitable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the logic of compute_index_put_ref
too complex? and it is better to use Numpy's fancy indexing, eg, x=np.transpose(x, perm); x[index] = value; x=np.transpose(x, perm)
be simpler and easier to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: zachary sun <70642955+sunzhongkai588@users.noreply.github.com>
Sorry to inform you that 59e28c3's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
请解决下冲突以及codestyle流水线的问题 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for docs
…addle#57416) * init index_fill API * modify api design * format code * optimize process * format code * adjust computation process * sample code format * fix code example * modify test reference * Update documentation Co-authored-by: zachary sun <70642955+sunzhongkai588@users.noreply.github.com> --------- Co-authored-by: zachary sun <70642955+sunzhongkai588@users.noreply.github.com>
…addle#57416) * init index_fill API * modify api design * format code * optimize process * format code * adjust computation process * sample code format * fix code example * modify test reference * Update documentation Co-authored-by: zachary sun <70642955+sunzhongkai588@users.noreply.github.com> --------- Co-authored-by: zachary sun <70642955+sunzhongkai588@users.noreply.github.com>
The index_fill and index_fill_ versions have different calculation results |
PR types
New features
PR changes
APIs
Description
Add index_fill/index_fill_ API to Paddle
Link
Rfc PR: PaddlePaddle/community#621
docs PR: PaddlePaddle/docs#6284
待完成