-
Notifications
You must be signed in to change notification settings - Fork 330
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
Updating FasterRCNN to use Task API #2012
base: master
Are you sure you want to change the base?
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.
Awesome start! Definitely will want to add presets as well. I'd expect we can just drop this model into our VOC OD training script and let it rip -- should be a good way to verify numerics!
Relevant issue: #1960 |
@ianstenbit ready for another round of review. |
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.
Looks very close -- but we still need tests!
The linter is also unhappy:
Copyright not found in keras_cv/models/object_detection/faster_rcnn/faster_rcnn_test.py
@ianstenbit I am ready for another set of reviews. I have also added the tests (these are the same as that of the legacy code). |
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.
Awesome progress!
It would be great to plug in FasterRCNN do this training script on Colab once we think it's ready to verify that it's training correctly.
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.
Awesome Aritra! I just have one more comment.
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.
Thank you I was giving it a once-over and noticed 2 missing tests
lets us add (adding reference from yolo_v8 here)
from keras_cv.backend import keras | ||
from keras_cv.backend import ops | ||
|
||
# from keras_cv.models.backbones.test_backbone_presets import ( |
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.
Hey @ariG23498! maybe you missed this part. Please add the test_backbone_presets
- an equivalent one in Yolo_v8 is here
boxes = y["boxes"] | ||
if len(y["classes"].shape) != 2: | ||
raise ValueError( | ||
"Expected 'classes' to be a tf.Tensor of rank 2. " |
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.
update tf.Tensor
to Tensor
((2, 512, 512, 3),), | ||
((2, 128, 128, 3),), | ||
) | ||
def test_faster_rcnn_train(self, batch_shape): |
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.
lets add a model.fit() to test training
from keras_cv.bounding_box.converters import _decode_deltas_to_boxes | ||
|
||
# from keras_cv.models.backbones.backbone_presets import backbone_presets | ||
# from keras_cv.models.backbones.backbone_presets import ( |
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.
remove commented imports
non max supression padded api from tf is yet to be ported
@ariG23498 one additional overhead work is needed. please add PS: we will fix this overhead soon, but in the mean time this is what we need to do to make sure the large GPU tests run. |
CC: @ianstenbit