Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[YOLOX] Step 2/? : Setting up YoloX structure, add internal layers and update iou losses #1296

Merged
merged 38 commits into from
Apr 11, 2023
Merged

[YOLOX] Step 2/? : Setting up YoloX structure, add internal layers and update iou losses #1296

merged 38 commits into from
Apr 11, 2023

Conversation

quantumalaviya
Copy link
Contributor

What does this PR do?

This PR is one of many upcoming PRs that will attempt to break down #982 so that it's easier to review. This one deals with the internal layers.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue? Please add a link
    to it if that's the case.
  • Did you write any new necessary tests?
  • If this adds a new model, can you run a few training steps on TPU in Colab to ensure that no XLA incompatible OP are used?

Who can review?

@LukeWood
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@sebastian-sz
Copy link
Contributor

Could we add names to layers? E.g.

for i in range(num_level):  # line 60 in yolox_head.py
# (...)
           # lines 109-128
            self.classification_preds.append(
                keras.layers.Conv2D(
                    filters=classes,
                    kernel_size=1,
                    strides=1,
                    padding="same",
                    bias_initializer=bias_initializer,
                    name=f"classification_{i}"
                )
            )
            self.regression_preds.append(
                keras.layers.Conv2D(
                    filters=4,
                    kernel_size=1,
                    strides=1,
                    padding="same",
                    bias_initializer=bias_initializer,
                    name=f"regression_{i}"
                )
            )

Semi-related to #1297

@quantumalaviya quantumalaviya mentioned this pull request Jan 21, 2023
Closed
LukeWood
LukeWood suggested changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@LukeWood LukeWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of review, thanks!

keras_cv/models/object_detection/yolox/__internal__/layers/yolox_decoder.py Outdated
)
self.built = True

def call(self, images, predictions):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets add some basic assertions around shape and raise a ValueError if the shapes are not what we expect.

len(images.shape) ==4, len(predictions.shape)== what you expect, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an internal layer. Is this required?

ianstenbit
ianstenbit suggested changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@ianstenbit ianstenbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
Have the pre-trained CSPDarkNet weights been okay so far for testing?

keras_cv/models/object_detection/yolox/__internal__/layers/yolox_head.py Outdated
classification and regression heads. Defaults to None.
width_multiplier: A float value used to calculate the base width of the model
this changes based on the detection model being used. Defaults to 1.0.
num_level: the number of levels in the FPN output. Defaults to 3.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (curious what @LukeWood thinks too).

Maybe let's call this levels? In general we've avoided the num_ prefix.
(Either way I think it should have an s at the end)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Waiting for @LukeWood here.

LukeWood
LukeWood suggested changes Jan 24, 2023
View reviewed changes
keras_cv/models/object_detection/yolox/__internal__/layers/yolox_head.py Outdated
boxes = self.regression_preds[i](boxes_feat)
objectness = self.objectness_preds[i](boxes_feat)

output = tf.keras.layers.Concatenate(axis=-1)([boxes, objectness, classes])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's a question: should we actually concat these or should we just pass a dictionary around?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for this specific use case, I think a concat is okay since it is only being used internally. I can refactor the code after YoloX is merged. But for now, I think we can let it be?

@quantumalaviya quantumalaviya changed the title [YOLOX] Step 2/? : Setting up YoloX structure and add internal layers [YOLOX] Step 2/? : Setting up YoloX structure, add internal layers and update iou losses Mar 22, 2023
@quantumalaviya quantumalaviya requested a review from LukeWood March 27, 2023 19:32
LukeWood
LukeWood reviewed Mar 28, 2023
View reviewed changes
keras_cv/models/object_detection/yolox/layers/yolox_decoder.py
from keras_cv import bounding_box


class YoloXPredictionDecoder(keras.layers.Layer):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way we can add some basic unit tests to this?

LukeWood
LukeWood suggested changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@LukeWood LukeWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Biggest concern is unit testing and numerical correctness. WDYT about testing just input/output shapes? My reasoning is this model is SO complex end to end - its good to rule out bug locations.

@quantumalaviya quantumalaviya requested a review from LukeWood April 3, 2023 17:54
LukeWood
LukeWood approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@LukeWood LukeWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great, thanks for your hard work. Let me give it one more check, then we can merge

@LukeWood
Copy link
Contributor

/gcbrun

@LukeWood LukeWood merged commit e889477 into keras-team:master Apr 11, 2023
ianstenbit added a commit that referenced this pull request Apr 13, 2023
@ianstenbit
Remove a redundant check from IoULoss
50803c0 
This was introduced in #1296 and is a duplicate of the check below which is more exhaustive.
@ianstenbit ianstenbit mentioned this pull request Apr 13, 2023
Merged
ianstenbit added a commit that referenced this pull request Apr 21, 2023
@ianstenbit
Remove a redundant check from IoULoss (#1707)
10f4ce7 
This was introduced in #1296 and is a duplicate of the check below which is more exhaustive.
ghost pushed a commit to y-vectorfield/keras-cv that referenced this pull request Nov 16, 2023
@quantumalaviya
[YOLOX] Step 2/? : Setting up YoloX structure, add internal layers an…
c4c2f0e 
…d update iou losses (keras-team#1296)

* first attempt at introducing YoloX

* formatted and fixed bugs

* cast fix keras-team#1

* cast fix keras-team#2

* cast fix keras-team#3

* cast fix keras-team#4

* adding ensure shape for support

* reverting and removing ensure_shape

* fixed another bug

* updated train.py

* updated docs, tests and added support for loss strings

* first attempt at introducing YoloX

* formatted and fixed bugs

* adding ensure shape for support

* reverting and removing ensure_shape

* reformatted by black

* fixed a  linting issue

* finally rebased atop the recent changes

* finally rebased atop the new changes

* fixed linting issues

* reverted rebasing issues with iou loss

* fixing rebased errors part 2

* fixed more linting issues

* TPU testing changes

* linting fixes

* updated with implementation details from paper

* updated based on review comments and api changes

* first attempt at introducing YoloX

* updated docs, tests and added support for loss strings

* fixed linting issues

* reverted rebasing issues with iou loss

* review comments

* removed examples

* linting fix

* fixed rebasing error

* updated no_reduction warning

* review comments

* revert version and linting fixes
ghost pushed a commit to y-vectorfield/keras-cv that referenced this pull request Nov 16, 2023
@ianstenbit
Remove a redundant check from IoULoss (keras-team#1707)
458c655 
This was introduced in keras-team#1296 and is a duplicate of the check below which is more exhaustive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianstenbit ianstenbit ianstenbit requested changes

@LukeWood LukeWood LukeWood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@quantumalaviya @sebastian-sz @LukeWood @ianstenbit