-
Notifications
You must be signed in to change notification settings - Fork 118
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
Convert RandomTranslation to the backend-agnostic implementation #572
Convert RandomTranslation to the backend-agnostic implementation #572
Conversation
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.
Thanks for the PR. It's looking good! Please make sure we have enough test coverage, I think the current unit tests are pretty weak. You can use pytest-cov
to check which parts of the code are covered.
self.width_lower, self.width_upper = self._set_factor( | ||
width_factor, "width_factor" | ||
) | ||
self._check_fill_mode_and_interpolation(fill_mode, interpolation) |
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.
You can inline this logic here, no need for a separate method since it's only used 1x
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
I have added some tests including:
Now the coverage becomes 100% Name Stmts Miss Cover
---------------------------------------------------------------------------
keras_core/layers/preprocessing/random_translation.py 74 0 100% |
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.
Excellent -- thank you for the contribution!
Related to keras-team/keras#18442
Tests all passed in my local env