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.

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

[Object_Detection] Add BoxCoder for SSD and FasterRCNN #4

Closed
wants to merge 4 commits into from

Conversation

tanzhenyu
Copy link
Contributor

Add BoxCoder for SSD and FasterRCNN.

@tanzhenyu tanzhenyu requested a review from fchollet June 16, 2020 22:52
@bhack
Copy link
Contributor

bhack commented Jun 17, 2020

Just as reminder we have a boxcoder in official/vision

@tanzhenyu
Copy link
Contributor Author

Just as reminder we have a boxcoder in official/vision

We do, this is to "graduate" that API, but as a Keras object

@bhack
Copy link
Contributor

bhack commented Jun 17, 2020

We do, this is to "graduate" that API, but as a Keras object

Ok I suppose that this "graduation" could be not API compatible with official/vision so how do you handle this in the spirit of the approved RFC already mentioned at tensorflow/community#223 (comment)?
Will be in voluntary/optional charge to the model garden maintainers to refactor after graduation? Will be part of the graduation process itself?
If not, will this target just t+1 models (so no official models refactoring)? Or will be reserved/mandatory only to models that will be contributed to keras-cv?

This just to understand the level of duplicates minimization that we are aspiring and future merging policy for models by an user/contributor point of view.

@tanzhenyu
Copy link
Contributor Author

We do, this is to "graduate" that API, but as a Keras object

Ok I suppose that this "graduation" could be not API compatible with official/vision so how do you handle this in the spirit of the approved RFC already mentioned at tensorflow/community#223 (comment)?
Will be in voluntary/optional charge to the model garden maintainers to refactor after graduation? Will be part of the graduation process itself?
If not, will this target just t+1 models (so no official models refactoring)? Or will be reserved/mandatory only to models that will be contributed to keras-cv?

This just to understand the level of duplicates minimization that we are aspiring and future merging policy for models by an user/contributor point of view.

So this is exactly being mentioned in:
https://github.com/tensorflow/community/blame/master/rfcs/20190802-model-garden-redesign.md#L60-L61

The tentative proposal is to have them in keras-CV, not addons, and can be contributed by model-garden team as well, so we wouldn't be having duplicating low-level APIs such as this in vision/. The proposal is that they will directly make a dependency on this, which is lightweight enough.

@bhack
Copy link
Contributor

bhack commented Jun 17, 2020

The tentative proposal is to have them in keras-CV, not addons, and can be contributed by model-garden team as well, so we wouldn't be having duplicating low-level APIs such as this in vision/. The proposal is that they will directly make a dependency on this, which is lightweight enough.

Yes but you have not replied on who is in charge of this process? In a similar case it seems to me more clear tensorflow/community#260 (comment)

@tanzhenyu
Copy link
Contributor Author

The tentative proposal is to have them in keras-CV, not addons, and can be contributed by model-garden team as well, so we wouldn't be having duplicating low-level APIs such as this in vision/. The proposal is that they will directly make a dependency on this, which is lightweight enough.

Yes but you have not replied on who is in charge of this process? In a similar case it seems to me more clear tensorflow/community#260 (comment)

We will probably release the finalized plan soon, which should answer your question. Stay tuned :-)

$ \hat{width_gt} = log(width_gt / width_a)
$ \hat{height_gt} = log(height_gt / height_a)

where cx, cy, width, height represents center of width, center of height, width, height respectively,

Choose a reason for hiding this comment

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

Do you have a coding style setting up? It looks this line is too long.

Copy link
Member

Choose a reason for hiding this comment

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

We should follow usual conventions and keep line length below 85 char. Note that black will not format comments/docstrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All that's why :-) Done

from kerascv.layers.ssd_box_coder import SSDBoxCoder


def test_encode_decode_variance():
Copy link

@saberkun saberkun Jun 19, 2020

Choose a reason for hiding this comment

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

Do we want to test all modes (e.g. graph, eager) of keras?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that testing util exposed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@fchollet fchollet 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 for the PR. Let's get a review from the model garden team as well so we can make sure we have a path forward for how this block will be used in the model garden codebase.

$ \hat{width_gt} = log(width_gt / width_a)
$ \hat{height_gt} = log(height_gt / height_a)

where cx, cy, width, height represents center of width, center of height, width, height respectively,
Copy link
Member

Choose a reason for hiding this comment

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

We should follow usual conventions and keep line length below 85 char. Note that black will not format comments/docstrings.

class SSDBoxCoder(tf.keras.layers.Layer):
"""Defines a SSDBoxEncoder that converts encodes the ground_truth_boxes using anchors.

Mathematically, the encoding is:
Copy link
Member

Choose a reason for hiding this comment

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

In general, don't use LaTeX in docstrings, as it is hard to read before rendering (and for many people even after rendering). Prefer using Python-like pseudocode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

`ground_truth_boxes` to anchors based on a certain matching strategy (argmax, bipartite)

# Attributes:
variances: The 1-D scaling factor with 4 floats. This is used to represent the variance of
Copy link
Member

Choose a reason for hiding this comment

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

It may be preferable to have 4 keyword arguments instead (or potentially 2 tuple arguments, one for the centers and one for the height/width), as it would avoid confusion regarding the order of the 4 floats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM for API

@tanzhenyu tanzhenyu closed this Sep 12, 2020
@tanzhenyu tanzhenyu deleted the box_coder branch September 17, 2020 22:43
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Oct 30, 2022
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Nov 3, 2022
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Jan 20, 2023
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Jan 29, 2023
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Feb 8, 2023
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Mar 22, 2023
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Mar 22, 2023
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Mar 27, 2023
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Mar 29, 2023
LukeWood pushed a commit that referenced this pull request Apr 11, 2023
…d update iou losses (#1296)

* first attempt at introducing YoloX

* formatted and fixed bugs

* cast fix #1

* cast fix #2

* cast fix #3

* cast fix #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
quantumalaviya added a commit to quantumalaviya/keras-cv that referenced this pull request Apr 24, 2023
tirthasheshpatel pushed a commit to tirthasheshpatel/keras-cv that referenced this pull request Jun 5, 2023
Port CSPDarkNet and DarkNet to Keras Core
ghost pushed a commit to y-vectorfield/keras-cv that referenced this pull request Nov 16, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants