keras-team
[YoloX Step 3/3] Add a TF implementation of YoloX to KerasCV
What does this PR do?
Adds YoloX-tiny, YoloX-s, YoloX-m, YoloX-l and YoloX-x to KerasCV. WIP. Also fixes IoU (#969) and adds GIoU loss.
Before submitting
- [x] Did you read the contributor guideline, Pull Request section?
- [x] 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?
- [ ] Write assertions and documentation to communicate the loss input and output shape expected by YoloX.
- [ ] Ensure that YoloX tests work properly (These are basically copied from retinanet tests and will need the accelerator to see if there are any errors)
Who can review?
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.
cc. @Joker316701882 @FateScript Mentioning some authors of the YOLOX to give some valuabe comments here. YOLOX-Docx. - YOLOX-Code
cc. @Joker316701882 @FateScript Mentioning some authors of the YOLOX to give some valuabe comments here. YOLOX-Docx. - YOLOX-Code
Any feedback from original authors would be awesome!
cc. @Joker316701882 @FateScript Mentioning some authors of the YOLOX to give some valuabe comments here. YOLOX-Docx. - YOLOX-Code
Any feedback from original authors would be awesome!
+100
@quantumalaviya Wow, it's really awesome !!! Thanks @quantumalaviya for your contribution and thanks @innat for mentioning me here. I will check the code and give my feed back soon !
This is definitely a great start! It shows how every stitches together with end to end workflow. However, as a side comment -- given how large this PR is, I'd strongly recommend splitting it into several PRs:
- provide CSPDarkNet implementation (or not if you decide to start with ResNet)
- provide base components in separate PRs, such as LabelEncoder in one PR, GIOU losses in one PR, etc
- provide the YOLOX implementation, start with only YOLOX tiny is good enough, with the training script
- provide improvement to the training script or fix bugs as follow-up PRs
This hopefully can help both the contributor in rebasing commits or merging conflicts, and help reviewers to focus on implementation detail.
Thanks for all the comments @tanzhenyu! I will separately go through them soon (hopefully over the weekend).
As of now, I see a lot of the comments are on the examples file. I understand that the example training script leaves a lot to be desired. Right now, it's just the sample script as copied from the retinanet training example and it doesn't have some very important YoloX implementation details such as Mosaic, Mixup, scheduler etc.
I can definitely break this PR down into multiple PRs. CSPDarkNet is the official backbone for YoloX so I am planning on using it. This has already been added to KerasCV in June. I will try to rewrite the training script soon and that should resolve a lot of the comments.
Thanks for all the comments @tanzhenyu! I will separately go through them soon (hopefully over the weekend).
As of now, I see a lot of the comments are on the examples file. I understand that the example training script leaves a lot to be desired. Right now, it's just the sample script as copied from the retinanet training example and it doesn't have some very important YoloX implementation details such as Mosaic, Mixup, scheduler etc.
I can definitely break this PR down into multiple PRs. CSPDarkNet is the official backbone for YoloX so I am planning on using it. This has already been added to KerasCV in June. I will try to rewrite the training script soon and that should resolve a lot of the comments.
Awesome, thanks!
Super cool! In the long run let's scope out rebasing on the new API style we've piloting for YOLOv8 and RetinaNet.
@jbischof I haven't had a chance to rebase this on the new API yet. However, the dependencies have already been merged to the main branch. Do you think we should remove those until I get a chance to work on rebasing this?
@jbischof I haven't had a chance to rebase this on the new API yet. However, the dependencies have already been merged to the main branch. Do you think we should remove those until I get a chance to work on rebasing this?
Are you referring to the dependent layers+losses? I think we should be able to just keep those around for now, but some of them will likely require porting to Keras Core (I didn't port the internal YOLOx layers, for example). Happy to help out with the porting effort, as I know YOLOx got caught in a weird spot with regard to the migration to Keras Core
