-
Notifications
You must be signed in to change notification settings - Fork 667
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
impl of reflection_pad1d and replication_pad1d #8724
Conversation
Flowingsun007
commented
Jul 21, 2022
•
edited
Loading
edited
- 重构pad2d_kernels.cpp,独立出reflection_pad_kernels、replication_pad_kernels
- 新增relfection_pad1d相关module、op/kernel、test case
- 新增replication_pad1d相关module、op/kernel、test case;by:@zhongshsh
Code got formatted by CI. Please request CI again if you still want to have this PR merged. If the PR is from a forked repo, please download the patch files from the GitHub Actions web page and apply them locally. |
…/oneflow into dev_support_reflection_pad1d
Code got formatted by CI. Please request CI again if you still want to have this PR merged. If the PR is from a forked repo, please download the patch files from the GitHub Actions web page and apply them locally. |
Speed stats:
|
Code got formatted by CI. Please request CI again if you still want to have this PR merged. If the PR is from a forked repo, please download the patch files from the GitHub Actions web page and apply them locally. |
View latest API docs preview at: https://staging.oneflow.info/docs/Oneflow-Inc/oneflow/pr/8724/ |
Speed stats:
|
View latest API docs preview at: https://staging.oneflow.info/docs/Oneflow-Inc/oneflow/pr/8724/ |
Speed stats:
|
CI failed when running job: cpu-module. PR label automerge has been removed |
Speed stats:
|
View latest API docs preview at: https://staging.oneflow.info/docs/Oneflow-Inc/oneflow/pr/8724/ |
Speed stats:
|
@@ -2147,25 +2147,31 @@ class PadFunctor { | |||
public: | |||
PadFunctor() { |
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.
这个函数实现得太糟糕了。一个函数那么长,嵌套了那么多的if。这是奔向屎山的节奏。
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.
这个函数实现得太糟糕了。一个函数那么长,嵌套了那么多的if。这是奔向屎山的节奏。
这个PadFunctor确实需要一些if判断,因为需要处理constant/reflection/replication pad的逻辑,以及对齐torch的相关check
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.
必须在意代码的篇幅,拆成小函数之后才好看清代码的逻辑。
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.
这个函数实现得太糟糕了。一个函数那么长,嵌套了那么多的if。这是奔向屎山的节奏。
这个PadFunctor确实需要一些if判断,因为需要处理constant/reflection/replication pad的逻辑,以及对齐torch的相关check
拆成小函数也不耽误这些逻辑的处理,也不会耽误对齐pytorch
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.
好的,我后面拆分一下