-
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
Move sgd to phi #40045
Move sgd to phi #40045
Conversation
… move_sgd_to_phi
… move_sgd_to_phi
… move_sgd_to_phi
Thanks for your contribution! |
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.
有几处细节问题,也可以后续PR完善
multi_precision ? master_param->data<MPDType>() : nullptr; | ||
MPDType* master_out_data = | ||
multi_precision | ||
? master_param_out->mutable_data<MPDType>(dev_ctx.GetPlace()) |
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.
mutable_data to Alloc
grad.data<T>(), | ||
learning_rate.data<T>(), | ||
param.numel(), | ||
param_out->mutable_data<T>(dev_ctx.GetPlace()), |
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.
同上
multi_precision ? master_param->data<MPDType>() : nullptr; | ||
MPDType* master_out_data = | ||
multi_precision | ||
? master_param_out->mutable_data<MPDType>(dev_ctx.GetPlace()) |
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.
同上
} | ||
|
||
template <typename T> | ||
void sgd_dense_param_sparse_grad_impl(const DenseTensor& param, |
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.
Optimizer相关的SelectedRows kernel是否要按照目前的规则放在selected_rows目录下?
// do check here | ||
// if (multi_precision) { | ||
// bool has_master = | ||
// ctx.HasInput("MasterParam") && ctx.HasOutput("MasterParamOut"); | ||
|
||
// } |
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.
注释是不是可以删除了?
// do some check here | ||
// if (multi_precision) { | ||
// bool has_master = | ||
// ctx.HasInput("MasterParam") && ctx.HasOutput("MasterParamOut"); | ||
|
||
// } |
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.
同上
PR types
Breaking changes
PR changes
OPs
Describe
move sgd to phi