Skip to content

Conversation

aquagull
Copy link
Contributor

@aquagull aquagull commented Nov 28, 2024

PR Category

User Experience

PR Types

New features

Description

【Paddle Tensor No.26】Svdvals new branch
#69082

Copy link

paddle-bot bot commented Nov 28, 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 Nov 28, 2024
@luotao1 luotao1 added the HappyOpenSource 快乐开源活动issue与PR label Nov 28, 2024
Comment on lines +72 to +74
@skip_check_grad_ci(
reason="'check_grad' on singular values is not required for svdvals."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

这个打开后在aistudio机器上要运行多久呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个打开后在aistudio机器上要运行多久呢?

这个还没测试过来着

Copy link
Contributor

@HydrogenSulfate HydrogenSulfate Nov 29, 2024

Choose a reason for hiding this comment

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

这个打开后在aistudio机器上要运行多久呢?

这个还没测试过来着

几百x几百规模的svd应该很快的,几十或者几百毫秒就能跑,你可以打开本地测试一下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个打开后在aistudio机器上要运行多久呢?

这个还没测试过来着

几百x几百规模的svd应该很快的,几十或者几百毫秒就能跑,你可以打开本地测试一下

好的我本地编译测试一下


np_s = np.linalg.svd(self.x_np, compute_uv=False, hermitian=False)
for r in res:
self.assertTrue(np.allclose(np_s, r, rtol=1e-6))
Copy link
Contributor

Choose a reason for hiding this comment

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

下个PR把所有的assertTrue(allclose)用np.testing.assert_allclose代替,这样错误信息会更清晰

Suggested change
self.assertTrue(np.allclose(np_s, r, rtol=1e-6))
np.testing.assert_allclose(np_s, r, rtol=1e-6))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

下个PR把所有的assertTrue(allclose)用np.testing.assert_allclose代替,这样错误信息会更清晰

了解

@SigureMo SigureMo requested a review from Copilot November 29, 2024 02:32
Comment on lines +2981 to +2994
if in_dynamic_or_pir_mode():
return _C_ops.svdvals(x)
else:
check_variable_and_dtype(x, 'dtype', ['float32', 'float64'], 'svdvals')
helper = LayerHelper('svdvals', **locals())
s = helper.create_variable_for_type_inference(dtype=x.dtype)
attrs = {}
helper.append_op(
type='svdvals',
inputs={'X': [x]},
outputs={'S': s},
attrs=attrs,
)
return s
Copy link
Contributor

Choose a reason for hiding this comment

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

下个PR再把老IR的分支删了

Suggested change
if in_dynamic_or_pir_mode():
return _C_ops.svdvals(x)
else:
check_variable_and_dtype(x, 'dtype', ['float32', 'float64'], 'svdvals')
helper = LayerHelper('svdvals', **locals())
s = helper.create_variable_for_type_inference(dtype=x.dtype)
attrs = {}
helper.append_op(
type='svdvals',
inputs={'X': [x]},
outputs={'S': s},
attrs=attrs,
)
return s
return _C_ops.svdvals(x)

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 17 changed files in this pull request and generated no suggestions.

Files not reviewed (12)
  • paddle/cinn/hlir/dialect/operator/ir/op_dialect.cc: Language not supported
  • paddle/fluid/pir/dialect/operator/ir/op_dialect.cc: Language not supported
  • paddle/fluid/pir/dialect/operator/ir/op_onednn_dialect.cc: Language not supported
  • paddle/phi/infermeta/unary.cc: Language not supported
  • paddle/phi/infermeta/unary.h: Language not supported
  • paddle/phi/kernels/cpu/svdvals_grad_kernel.cc: Language not supported
  • paddle/phi/kernels/cpu/svdvals_kernel.cc: Language not supported
  • paddle/phi/kernels/impl/svdvals_grad_kernel_impl.h: Language not supported
  • paddle/phi/kernels/svdvals_grad_kernel.h: Language not supported
  • paddle/phi/kernels/svdvals_kernel.h: Language not supported
  • paddle/phi/ops/yaml/backward.yaml: Evaluated as low risk
  • paddle/phi/ops/yaml/ops.yaml: Evaluated as low risk

Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTMeow 🐾

@@ -0,0 +1,158 @@
# Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
# Copyright (c) 2024 PaddlePaddle Authors. All Rights Reserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我在下一个pr里更改吧

Comment on lines +2399 to +2403
n = len(op_list_strs) // 4
first_part_op_info = op_list_strs[:n]
second_part_op_info = op_list_strs[n:]
second_part_op_info = op_list_strs[n : 2 * n]
third_part_op_info = op_list_strs[2 * n : 3 * n]
fourth_part_op_info = op_list_strs[3 * n :]
Copy link
Member

Choose a reason for hiding this comment

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

n_parts = 4
part_size = math.ceil(len(op_list_strs) / n_part)
op_list_parts = [op_list_strs[part_offset:part_offset + part_size] for part_offset in range(0, len(op_list_strs), part_size)]

后续可以考虑以更易于维护的方式来扩展

Copy link
Contributor Author

Choose a reason for hiding this comment

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

n_parts = 4
part_size = math.ceil(len(op_list_strs) / n_part)
op_list_parts = [op_list_strs[part_offset:part_offset + part_size] for part_offset in range(0, len(op_list_strs), part_size)]

后续可以考虑以更易于维护的方式来扩展

好的谢谢佬

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

@HydrogenSulfate HydrogenSulfate merged commit 54ec035 into PaddlePaddle:develop Nov 29, 2024
28 checks passed
@aquagull aquagull deleted the svdvals_new_branch branch November 29, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers HappyOpenSource 快乐开源活动issue与PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants