-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
support adagrad sparse update #5272
Conversation
paddle/operators/adagrad_op.cc
Outdated
@@ -13,6 +13,8 @@ See the License for the specific language governing permissions and | |||
limitations under the License. */ | |||
|
|||
#include "paddle/operators/adagrad_op.h" | |||
#include <cmath> | |||
#include "paddle/operators/math/selected_rows_functor.h" |
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.
Does it make sense to place empty lines before and after #include cmath by following http://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes ?
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.
Done
@@ -29,25 +37,43 @@ class AdagradOpKernel : public framework::OpKernel<T> { | |||
param_out_tensor->mutable_data<T>(ctx.GetPlace()); | |||
moment_out_tensor->mutable_data<T>(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.
Does it make sense to move lines 26 - 30 into the if else block since they are only used for LoDTensor type?
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.
Actually, we have to allocate memory before calculating. param_out_tensor->mutable_data<T>(ctx.GetPlace());
have to be called in both if-else block. I will unify the variable name in both if-else block to avoid inconsistence.
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.
LGTM!
Fix #5271