Skip to content

Conversation

Enigmatisms
Copy link
Contributor

@Enigmatisms Enigmatisms commented Aug 11, 2025

PR Category

Operator Mechanism

PR Types

New features

Description

本 PR 为 #74495 的 reopen 版本,rebase 到了一个更新的版本,并且解决了与 #74506 的冲突。#74495 当时只用于CI发现问题,本 PR 尝试对其中“共用amin/amax backward op 但amin/amax 不支持某些整数类型“的问题进行了修复,基于SFINAE与python端检查。目前本 PR 在 #74506 未合入前会显得改动过多,实际上是包含了部分前序 PR 的改动,前序 PR merge 后应该可以自动 resolve。

本 PR 新增的 feature:

  • 两个新的 PHI kernel:(min/max)_with_index,底层基于 cub 的 Argmin/Argmax 操作,同时 reduce key/value。此 PHI kernel 仅在 GPU 端进行了支持。评估:
    • CPU 端的 argmin/argmax 操作基于 Eigen::TensorMap 提供的 argmin/argmax 操作,无法对 value 同时进行 reduce。
    • 其他后端暂不考虑支持。
  • 新的 PHI kernel 的反向传播算子:(min/max)_with_index_grad注意⚠️:此 backward 行为与 torch documentation 不一致!原因:torch 的文档错了,见我给 PyTorch 提的这个 issue:Torch Issue 160273,其行为根本不是 amin/amax 的行为:只对minimum/maximum index位置的结果传梯度,像是对梯度进行了一个 take_along_axis。
  • 其他与新算子相关的支持(如InferMeta/InferMetaSymbolic)
  • 两个新增的操作:paddle.compat.min, paddle.compat.max,与 torch 的行为进行对齐。

torch.min/torch.max 输入输出关系很复杂(一个API包含了太多功能):

  • 单输入时是全 reduce:仅输出 tensor
  • 输入 dim/keepdim 时:输出 values/indices
  • 输入 other 时:行为与 minimum/maximum 一致

除上述【情况2】在 CUDA GPU 后端下会调用 (min/max)_with_index,其余情况都是由 python 调用 _C_ops.xxx 获得结果的。其中情况1/2/3 在CUDA GPU后端下应该都具有较好的性能(没有进行组合,调用单算子完成),而情况2在其他后端下使用 argmin/max 与 take_along_axis 组合(并且需要配合 squeeze_ 操作),不是最优性能方案,但应当具有较高的开发性价比。

TODO
  • 本地 25个简单单测(单算子不同的输入组合)
  • PaConvert 库测试(14/14)。
  • PHI kernel 相关的单测(比如静态图 OpPass 单测等等)
  • test_compat_minmax.py: 等能达到单测覆盖率要求的算子单测:coverage 覆盖率可能显示有误。
  • 算子python函数内英文文档。

Pcard-89620

Copy link

paddle-bot bot commented Aug 11, 2025

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

@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2025

Codecov Report

❌ Patch coverage is 78.19149% with 41 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (develop@7731d1c). Learn more about missing BASE report.

Files with missing lines Patch % Lines
.../interface/infer_symbolic_shape/unary_infer_sym.cc 0.00% 24 Missing ⚠️
python/paddle/tensor/compat.py 90.07% 13 Missing ⚠️
python/paddle/tensor/math.py 33.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop   #74547   +/-   ##
==========================================
  Coverage           ?   78.19%           
==========================================
  Files              ?        5           
  Lines              ?      188           
  Branches           ?        0           
==========================================
  Hits               ?      147           
  Misses             ?       41           
  Partials           ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Enigmatisms Enigmatisms force-pushed the min_max_api_new branch 3 times, most recently from ff2ded8 to 6414203 Compare August 19, 2025 08:21
@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

@Enigmatisms
Copy link
Contributor Author

/re-run CI-Build

@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

2 similar comments
@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

zhangbo9674
zhangbo9674 previously approved these changes Aug 22, 2025
Copy link
Contributor

@zhangbo9674 zhangbo9674 left a comment

Choose a reason for hiding this comment

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

LGTM

zyfncg
zyfncg previously approved these changes Aug 22, 2025
XiaoguangHu01
XiaoguangHu01 previously approved these changes Aug 22, 2025
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@Enigmatisms Enigmatisms dismissed stale reviews from XiaoguangHu01, zyfncg, and zhangbo9674 via 0fbbb99 August 22, 2025 17:13
@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

2 similar comments
@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

Copy link
Contributor

@wanghuancoder wanghuancoder left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zhangbo9674 zhangbo9674 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +580 to +584
- For case 1: a single value Tensor (0-dim)
- For case 2: a named tuple MinMaxRetType(values: Tensor, indices: Tensor), `values` has the same data type as the `input`,
while indices is always an int64 Tensor, with exactly the same shape as `values`.
MinMaxRetType can be used (indexed, packed, unpacked) in the same way as a regular tuple
- For case 3: see `paddle.minimum`
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
- For case 1: a single value Tensor (0-dim)
- For case 2: a named tuple MinMaxRetType(values: Tensor, indices: Tensor), `values` has the same data type as the `input`,
while indices is always an int64 Tensor, with exactly the same shape as `values`.
MinMaxRetType can be used (indexed, packed, unpacked) in the same way as a regular tuple
- For case 3: see `paddle.minimum`
- For case 1. A single value Tensor (0-dim)
- For case 2. A named tuple MinMaxRetType(values: Tensor, indices: Tensor), `values` has the same data type as the `input`,
while indices is always an int64 Tensor, with exactly the same shape as `values`.
MinMaxRetType can be used (indexed, packed, unpacked) in the same way as a regular tuple
- For case 3. See `paddle.minimum`(:ref:`api_paddle_minimum`)
  • Returns内容里,引号:前面的内容会自动渲染成return type,所以尽量不要用引号
image
  • 加一下case3 api的跳转引用

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,补PR进行修复。

) -> Tensor | MinMaxRetType:
"""

Computes the minimum of tensor elements. There are mainly 3 cases (functionalities):
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
Computes the minimum of tensor elements. There are mainly 3 cases (functionalities):
Computes the minimum of tensor elements. There are mainly 3 cases (functionalities):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,补PR进行修复。

Comment on lines +551 to +552
Special warning: the gradient behavior is NOT well-documented by PyTorch, the actual behavior should be:
1. Case 1: the same as `min`
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
Special warning: the gradient behavior is NOT well-documented by PyTorch, the actual behavior should be:
1. Case 1: the same as `min`
Special warning: the gradient behavior is NOT well-documented by PyTorch, the actual behavior should be:
1. Case 1: the same as `min`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,补PR进行修复。

) -> Tensor | MinMaxRetType:
"""

Computes the maximum of tensor elements. There are mainly 3 cases (functionalities):
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
Computes the maximum of tensor elements. There are mainly 3 cases (functionalities):
Computes the maximum of tensor elements. There are mainly 3 cases (functionalities):

Comment on lines +703 to +704
Special warning: the gradient behavior is NOT well-documented by PyTorch, the actual behavior should be:
1. Case 1: the same as `max`
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
Special warning: the gradient behavior is NOT well-documented by PyTorch, the actual behavior should be:
1. Case 1: the same as `max`
Special warning: the gradient behavior is NOT well-documented by PyTorch, the actual behavior should be:
1. Case 1: the same as `max`

Comment on lines +731 to +736
Returns:
- For case 1: a single value Tensor (0-dim)
- For case 2: a named tuple MinMaxRetType(values: Tensor, indices: Tensor), `values` has the same data type as the `input`,
while indices is always an int64 Tensor, with exactly the same shape as `values`.
MinMaxRetType can be used (indexed, packed, unpacked) in the same way as a regular tuple
- For case 3: see `paddle.maximum`
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
Returns:
- For case 1: a single value Tensor (0-dim)
- For case 2: a named tuple MinMaxRetType(values: Tensor, indices: Tensor), `values` has the same data type as the `input`,
while indices is always an int64 Tensor, with exactly the same shape as `values`.
MinMaxRetType can be used (indexed, packed, unpacked) in the same way as a regular tuple
- For case 3: see `paddle.maximum`
Returns:
- For case 1. A single value Tensor (0-dim)
- For case 2. A named tuple MinMaxRetType(values: Tensor, indices: Tensor), `values` has the same data type as the `input`,
while indices is always an int64 Tensor, with exactly the same shape as `values`.
MinMaxRetType can be used (indexed, packed, unpacked) in the same way as a regular tuple
- For case 3. See `paddle.maximum` (:ref:`api_paddle_maximum`)

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.

文档问题稍后修改~

@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

1 similar comment
@Enigmatisms
Copy link
Contributor Author

/re-run all-failed

@zhangbo9674 zhangbo9674 merged commit b95cb68 into PaddlePaddle:develop Aug 25, 2025
151 of 168 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants