-
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
Adding the FTRL optimizer. #5785
Conversation
if lr_power == -0.5: | ||
y = (np.sqrt(new_accum) / lr) + (2 * l2) | ||
pre_shrink = x / y | ||
param_out = np.where(linear_out > l1, pre_shrink, 0) |
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.
I compared the result with the tensorflow, this part is wrong.
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.
Oh I see, yes the C++ code is correct and is same as Tensorflow. Let me check the python code again.
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.
I commented out the part for calculating param_out
and even then, the computation of linear_out
itself doesn't match the C++ output. I think it could be a precision error, I can try once with double.
|
||
namespace ops = paddle::operators; | ||
REGISTER_OP_GPU_KERNEL(ftrl, | ||
ops::FTRLOpKernel<paddle::platform::GPUPlace, float>); |
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.
The calculations here not sufficiently precise in float32. This is not a bug, but we need to consider support double, fp16.
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.
And the same with attribute types.
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.
Yeah that's a good point.
@dzhwinter I have updated the code to work and pass the test case now. |
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!
Adding the FTRL optimizer