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
Refactor SparseApplyFtrl CPU kernel into class #43868
Refactor SparseApplyFtrl CPU kernel into class #43868
Conversation
- This is in preparation for adding a GPU implementation.
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.
Minor comment inline.
I'm assuming this PR is NFC (no functional change); if yes then please change the description to say so, if not, please say that as well.
Description edited to describe NFC. |
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.
Minor comment inline, lgtm other than that.
OP_REQUIRES( | ||
ctx, bad_i < 0, | ||
errors::InvalidArgument( | ||
"indices", SliceDebugString(indices.shape(), bad_i), " = ", | ||
indices_vec(bad_i), " is not in [0, ", var.dim_size(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.
IMO making this non local is harder to understand. Can you have operatior()
return a Status
(OK
or InvalidArgument
) and use OP_REQUIRES_OK
here?
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
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
This is in preparation for adding a GPU implementation.
(This is the second split of #43299).
cc @nluehr @sanjoy
Edit: This PR makes no functional change except for the indexing error message (which is changed to match that of SparseApplyKerasMomentumOp which has the same code structure).