Remove bounding box utils and refactor retinanet #2039

sineeli · 2025-01-08T23:26:03Z

Moved non_max_suppression, anchor_generator, and box_matcher into the modeling layers for better integration.
Removed redundant bounding box utilities and dependencies

mattdangerw · 2025-01-13T22:07:02Z

Not sure what best to do about this test failure. @sineeli do you know the minimal version of Keras we would need for all of this?

sineeli · 2025-01-13T22:12:57Z

Not sure what best to do about this test failure. @sineeli do you know the minimal version of Keras we would need for all of this?

It would require latest release 3.8.0 which has iou function and box encoding utilities as well.

mattdangerw

Thanks! Left some comments

keras_hub/src/layers/modeling/non_max_supression.py

mattdangerw · 2025-01-13T22:08:37Z

keras_hub/src/layers/modeling/anchor_generator.py


 @keras_hub_export("keras_hub.layers.AnchorGenerator")
 class AnchorGenerator(keras.layers.Layer):
    """Generates anchor boxes for object detection tasks.
-


these look like copy paste bugs where selecting from github removes empty newlines. bring these empty newlines back

This is unfixed. Take a look at the diff here and restore the empty newlines.

If you copy paste from the github ui you will remove all empty newlines from the file, just something to watch out for.

keras_hub/src/layers/modeling/box_matcher.py

keras_hub/src/layers/modeling/anchor_generator.py

keras_hub/src/layers/modeling/non_max_supression.py

keras_hub/src/models/retinanet/retinanet_image_converter.py

keras_hub/src/layers/modeling/anchor_generator.py

mattdangerw

Still a few comments to address. The big one, we need to get rid of the private import of keras for that validation function. Let's remove or duplicate.

We might also want to set things up so the library still works with older keras versions, and throws an error when using an OD model that 3.8 is required. But I can help with that part.

keras_hub/src/models/retinanet/retinanet_image_converter.py

keras_hub/src/layers/modeling/non_max_supression.py

sineeli · 2025-01-21T22:06:23Z

Oh yeah make sense, we can only import when we have Keras>=3.8.0 version and then throw error when this doesn't satisfy? I hope this works.

keras_hub/src/layers/modeling/non_max_supression.py

keras_hub/api/layers/__init__.py

keras_hub/src/layers/modeling/non_max_supression.py

mattdangerw · 2025-01-27T21:16:47Z

keras_hub/src/models/retinanet/retinanet_object_detector.py

@@ -18,6 +15,10 @@
    RetinaNetObjectDetectorPreprocessor,
 )

+# Check if Keras version is greater than or equal to 2.10.0
+if version.parse(keras.__version__) < version.parse("3.8.0"):


This is not how we can do this. We need this library to be importable with older versions of Keras. And this file will be imported, and this code block run, on library import!

We need to instead assert the version on usage of symbols we care about.

Add a function here called assert_bounding_box_support().

Call it here inside init, first thing. Also add the same check to anchor generator and box matcher layers.

mattdangerw · 2025-01-27T21:17:25Z

keras_hub/src/models/retinanet/retinanet_object_detector.py

+# Check if Keras version is greater than or equal to 2.10.0
+if version.parse(keras.__version__) < version.parse("3.8.0"):
+    raise ImportError("Requires 3.8.0 or higher.")
+

 @keras_hub_export("keras_hub.models.RetinaNetObjectDetector")
 class RetinaNetObjectDetector(ImageObjectDetector):


We need to align on names here. Either ImageObjectDetector and RetinaNetImageObjectDetector, or ObjectDetector and RetinaNetObjectDetector. That's our general pattern.

I'd prefer just rename ImageObjectDetector -> ObjectDetector everywhere. cc @divyashreepathihalli

mattdangerw · 2025-01-27T21:21:27Z

keras_hub/src/models/retinanet/retinanet_image_converter.py

@@ -10,25 +12,36 @@ class RetinaNetImageConverter(ImageConverter):

    def __init__(
        self,
-        image_size=None,


This seems suspicious. Why are we removing these args? Keep these unaltered, add bounding_box_format after.

sineeli · 2025-01-30T21:49:17Z

@mattdangerw what do you think on assert_bounding_box_support function, does it looks good or else any changes needed please let me know thanks!

mattdangerw

Thanks! Looking good. Still a few minor things.

mattdangerw · 2025-01-31T21:14:51Z

keras_hub/src/layers/modeling/anchor_generator.py


 @keras_hub_export("keras_hub.layers.AnchorGenerator")
 class AnchorGenerator(keras.layers.Layer):
    """Generates anchor boxes for object detection tasks.
-


This is unfixed. Take a look at the diff here and restore the empty newlines.

If you copy paste from the github ui you will remove all empty newlines from the file, just something to watch out for.

mattdangerw · 2025-01-31T21:24:47Z

keras_hub/src/layers/modeling/anchor_generator_test.py

 from keras_hub.src.tests.test_case import TestCase


 class AnchorGeneratorTest(TestCase):
+    @pytest.mark.skipif(


You do a class level skipif later it looks like. Why not do that here? You are skipping every test.

mattdangerw · 2025-01-31T21:25:21Z

keras_hub/src/layers/modeling/box_matcher_test.py


-from keras_hub.src.models.retinanet.box_matcher import BoxMatcher
+from keras_hub.src.layers.modeling.box_matcher import BoxMatcher
 from keras_hub.src.tests.test_case import TestCase


 class BoxMatcherTest(TestCase):


Same here, why not do this at the class level if you are skipping everything in the class.

mattdangerw · 2025-01-31T21:36:24Z

keras_hub/src/utils/tensor_utils.py

@@ -262,6 +264,15 @@ def assert_tf_libs_installed(symbol_name):
        )


+def assert_bounding_box_support(symbol_name):
+    keras_version = re.sub(r".dev.*", "", keras.__version__)


why do you need this line? version should handle dev release markers and other stuff here https://packaging.python.org/en/latest/discussions/versioning/

mattdangerw · 2025-01-31T21:37:31Z

keras_hub/src/models/retinanet/retinanet_label_encoder.py

            anchor_boxes,
            gt_boxes,
            bounding_box_format=self.bounding_box_format,
-            image_shape=image_shape,
+            image_shape=(height, width, 3),


Looks like the RetinaNetBackbone allows passing the number of channels, but here we are hardcoding it. Isn't that a bug?

mattdangerw · 2025-01-31T21:53:50Z

keras_hub/src/models/retinanet/retinanet_image_converter.py

@@ -40,14 +59,15 @@ def call(self, inputs):
        if self.norm_std:
            x = x / self._expand_non_channel_dims(self.norm_std, x)

-        return x
+        return x, y, sample_weight


call pack_x_y_sample_weight here, it will gracefully handle the no label case.

mattdangerw · 2025-01-31T21:56:19Z

keras_hub/src/models/retinanet/retinanet_image_converter.py

-        # TODO: https://github.com/keras-team/keras-hub/issues/1965
-        x = inputs
+    def call(self, x, y=None, sample_weight=None):
+        if y is not None:


I don't think we need two resizing layers here (which is also just kind of confusing).

Wouldn't it work to make one self.resizing layer above, and then...

if y is not None: inputs = self.resizing({"images": x, "bounding_boxes": y}) x = inputs["images"] y = inputs["bounding_boxes"] else: x = self.resizing(x)

remove bounding box utils and refactor retinanet

a83d9f9

sineeli requested a review from mattdangerw January 9, 2025 16:27

mattdangerw reviewed Jan 13, 2025

View reviewed changes

sineeli added 2 commits January 13, 2025 15:07

Remove bbox utils from KerasHub, adding proper docstrings

43e4591

nit

feffff6

mattdangerw reviewed Jan 21, 2025

View reviewed changes

keras_hub/src/models/retinanet/retinanet_image_converter.py Outdated Show resolved Hide resolved

keras_hub/src/models/retinanet/retinanet_image_converter.py Outdated Show resolved Hide resolved

keras_hub/src/layers/modeling/non_max_supression.py Outdated Show resolved Hide resolved

divyashreepathihalli reviewed Jan 22, 2025

View reviewed changes

keras_hub/src/layers/modeling/non_max_supression.py Outdated Show resolved Hide resolved

sineeli added 2 commits January 23, 2025 11:41

Import obj_det when satisfies keras version and nit

71c560b

Merge branch 'master' into retinanet-refactor

fc3f101

mattdangerw reviewed Jan 27, 2025

View reviewed changes

sineeli added 4 commits January 27, 2025 14:14

Correct api names and use assert for boudning box support

9fa1e57

Merge branch 'master' into retinanet-refactor

99c6afd

fix resize error and skip tests version < 3.8.0

26e34d8

fix tuple error for skip test cases

f91c6d5

sineeli requested a review from mattdangerw January 30, 2025 21:49

sineeli added 2 commits January 30, 2025 14:16

skip retinanet test cases for keras < 3.8.0

9c64c1f

correct re string value

d11d8d3

sineeli added the kokoro:force-run Runs Tests on GPU label Jan 30, 2025

kokoro-team removed the kokoro:force-run Runs Tests on GPU label Jan 30, 2025

mattdangerw reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove bounding box utils and refactor retinanet #2039

Remove bounding box utils and refactor retinanet #2039

sineeli commented Jan 8, 2025

mattdangerw commented Jan 13, 2025 •

edited

Loading

sineeli commented Jan 13, 2025

mattdangerw left a comment

mattdangerw Jan 13, 2025

sineeli Jan 13, 2025

mattdangerw Jan 31, 2025

mattdangerw left a comment

sineeli commented Jan 21, 2025

mattdangerw Jan 27, 2025

mattdangerw Jan 27, 2025

mattdangerw Jan 27, 2025

sineeli commented Jan 30, 2025 •

edited

Loading

mattdangerw left a comment

mattdangerw Jan 31, 2025

mattdangerw Jan 31, 2025

mattdangerw Jan 31, 2025

mattdangerw Jan 31, 2025

mattdangerw Jan 31, 2025

mattdangerw Jan 31, 2025

mattdangerw Jan 31, 2025

Remove bounding box utils and refactor retinanet #2039

Are you sure you want to change the base?

Remove bounding box utils and refactor retinanet #2039

Conversation

sineeli commented Jan 8, 2025

mattdangerw commented Jan 13, 2025 • edited Loading

sineeli commented Jan 13, 2025

mattdangerw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdangerw left a comment

Choose a reason for hiding this comment

sineeli commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sineeli commented Jan 30, 2025 • edited Loading

mattdangerw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdangerw commented Jan 13, 2025 •

edited

Loading

sineeli commented Jan 30, 2025 •

edited

Loading