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

add cumprod_grad composite #64432

Merged
merged 11 commits into from
May 24, 2024
Merged

Conversation

YibinLiu666
Copy link
Contributor

@YibinLiu666 YibinLiu666 commented May 19, 2024

PR Category

Others

PR Types

New features

Description

基于支持了 inclusive, reverse 参数的 cumprod 算子(#64022),实现 cumprod_grad 的组合逻辑,避免 cumprod_grad 的组合算子因为输入含有 0 而导致输出除 0 出 nan 的问题

Copy link

paddle-bot bot commented May 19, 2024

你的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 May 19, 2024
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate left a comment

Choose a reason for hiding this comment

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

辛苦提交PR,单测和代码可以再看一下

Comment on lines 1089 to 1090
auto ones_tensor = full<T>(x_dim, 1.0, x.dtype());
auto replace_one = (1 - zero_mask) * x + zero_mask * ones_tensor;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 加法第二项zero_mask * ones_tensor是不是可以化简为zero_mask,一个张量乘以全1矩阵ones_tensor等于没乘?
  2. 如果上面结论成立,可以删除ones_tensor

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

Comment on lines 24 to 36
@param.parameterized_class(
('primal', 'dtype'),
[
(
np.random.rand(2, 3, 4),
np.float32,
),
(
np.random.rand(2, 3, 3, 4),
np.float32,
),
],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 形状可以添加0D、1D、2D、5D
  2. 数据类型添加int64
  3. 数据本身添加包含1个0、2个0,全部都是0的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

Copy link
Contributor

Choose a reason for hiding this comment

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

补充更多单测case,同test/prim/prim/vjp/eager/test_comp_eager_cumprod_grad.py

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

Comment on lines 1087 to 1088
auto zero_mask_cumsum1 = cumsum<T>(zero_mask, dim, false, false, reverse);
auto zero_mask_cumsum2 = cumsum<T>(zero_mask, dim, false, true, reverse);
Copy link
Contributor

Choose a reason for hiding this comment

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

变量命名尽量准确符合语义,
zero_mask_cumsum1 --> zero_mask_cumsum_left
zero_mask_cumsum2 --> zero_mask_cumsum_right

Comment on lines 77 to 78
auto ones_tensor = full<T>(x_dim, 1.0, x.dtype());
auto replace_one = (1 - zero_mask) * x + zero_mask * ones_tensor;
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的ones_tensor应该可以删掉吧

Comment on lines 77 to 78
auto ones_tensor = full<T>(x_dim, 1.0, x.dtype());
auto replace_one = (1 - zero_mask) * x + zero_mask * ones_tensor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto ones_tensor = full<T>(x_dim, 1.0, x.dtype());
auto replace_one = (1 - zero_mask) * x + zero_mask * ones_tensor;
auto replace_one = (1 - zero_mask) * x + zero_mask;

Comment on lines 77 to 78
auto ones_tensor = full<T>(x_dim, 1.0, x.dtype());
auto replace_one = (1 - zero_mask) * x + zero_mask * ones_tensor;
Copy link
Contributor

Choose a reason for hiding this comment

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

逻辑连续的代码块之间可以加上一些注释,比如此处将0的位置填充成1,可以加上: # fill the positions of 0 with 1.

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


@param.parameterized_class(
('primal', 'dtype'),
[
Copy link
Contributor

Choose a reason for hiding this comment

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

加上一个单元素的测试:np.array(np.rand(), dtype="float32")

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

return out


@param.parameterized_class(
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate May 21, 2024

Choose a reason for hiding this comment

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

同上,补充单元素的0-Dcase

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

Copy link
Contributor

Choose a reason for hiding this comment

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

此处的实现跟details.h里的实现好像有一些区别?可以确认一下

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

Comment on lines 1090 to 1093
auto zero_mask_cumsum_exclusive =
cumsum<T>(zero_mask, dim, false, true, reverse);
auto zero_mask_cumsum =
zero_mask_cumsum_inclusive + zero_mask_cumsum_exclusive;
Copy link
Contributor

@HydrogenSulfate HydrogenSulfate May 22, 2024

Choose a reason for hiding this comment

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

inclusive + exclusive 是否相当于两倍的exclusive + x?两次cumprod应该比一次cumprod + scale + add耗时会更长,是否可以改成后者实现方式?
zero_mask_cumsum_inclusive + zero_mask_cumsum_exclusive --> scale<T>(zero_mask_cumsum_exclusive, 2) + zero_mask

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

Copy link
Contributor

@cyber-pioneer cyber-pioneer left a comment

Choose a reason for hiding this comment

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

todo: 建议适配optest,当前的这批单侧属于遗留case,未来会被清理

@HydrogenSulfate HydrogenSulfate merged commit 4b757dd into PaddlePaddle:develop May 24, 2024
32 checks passed
@YibinLiu666 YibinLiu666 deleted the cumprod_grad branch May 24, 2024 05:12
chen2016013 pushed a commit to chen2016013/Paddle that referenced this pull request May 26, 2024
* add cumprod_grad composite

* remove cout

* update test and fix some bug

* update test

* add comment and test

* remove cout

* update

* update static test

* update

* Update details.h
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.

5 participants