-
Notifications
You must be signed in to change notification settings - Fork 328
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
Vectorized Random Shear. #1537
Vectorized Random Shear. #1537
Conversation
The test of a different layer fails, will update tomorrow. |
3b34933
to
54b5e21
Compare
LGTM @sebastian-sz but can you rebase on #1534 and include results from the demo? |
54b5e21
to
f431406
Compare
@LukeWood I'm getting errors after rebase. There seems to be an issue when calling The error is thrown in: keras-cv/keras_cv/bounding_box/converters.py Line 230 in 1974263
Standalone code to reproduce: import demo_utils
import keras_cv.bounding_box
dataset = demo_utils.load_voc_dataset(bounding_box_format="xyxy")
for d in dataset.take(1):
d["bounding_boxes"] = keras_cv.bounding_box.to_dense(
d["bounding_boxes"], default_value=0
)
keras_cv.bounding_box.convert_format(
boxes=d["bounding_boxes"],
source="xyxy",
target="rel_xyxy",
images=d["images"],
) The error goes away if I comment out the Ragged check: keras-cv/keras_cv/bounding_box/converters.py Lines 514 to 516 in 1974263
# if isinstance(boxes, tf.RaggedTensor):
height = tf.expand_dims(height, axis=-1)
width = tf.expand_dims(width, axis=-1) but this might break something somewhere else. Not sure how to proceed with this. |
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!
This LGTM -- @LukeWood can you PTAL at the demo issue?
) | ||
from keras_cv.utils import preprocessing | ||
|
||
|
||
@keras.utils.register_keras_serializable(package="keras_cv") | ||
class RandomShear(BaseImageAugmentationLayer): | ||
@tf.keras.utils.register_keras_serializable(package="keras_cv") |
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.
nit: let's use keras
directly and add from tensorflow import keras
above
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.
@ianstenbit Done.
looks like an unrelated error. I will file a bug for this converter issue, in the meantime can you change the bounding box format used in the demo in a way to avoid this issue (if not possible no worries) |
@LukeWood commented out the Ragged check. Note that both layers sample different shear values. |
Thanks @sebastian-sz ! |
* Vectorized Random Shear. * Fixed failing tests. * Refactored transformarion matrix creation. * Remove unused import. * Replaced tf.keras with keras.
What does this PR do?
Refactors RandomShear to subclass
VectorizedBaseImageAugmentationLayer
.This gives performance benefits.
Fixes #1518
I had little error in my previous benchmark. The correct performance numbers are still better but the difference between implementations is smaller.
Before submitting
Pull Request section?
to it if that's the case.
Who can review?
@LukeWood @ianstenbit