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

Improve yolox-tiny tile recipe #3767

Conversation

eugene123tw
Copy link
Contributor

@eugene123tw eugene123tw commented Jul 25, 2024

Summary

Model Val Input Size Dataset mAP
YOLOX-TINY 640 x 640 Vitens Coliform 0.4916
YOLOX-TINY 640 x 640 Vitens Aeromonas 0.8870
YOLOX-TINY 640 x 640 PACKAGE 0.8699
YOLOX-TINY 640 x 640 WGISD 0.6467
YOLOX-TINY 640 x 640 blueberry 0.8375
Model Val Input Size Dataset mAP
YOLOX-TINY 416 x 416 Vitens Coliform 0.3994
YOLOX-TINY 416 x 416 Vitens Aeromonas 0.7575
YOLOX-TINY 416 x 416 PACKAGE 0.8565
YOLOX-TINY 416 x 416 WGISD 0.6185
YOLOX-TINY 416 x 416 blueberry 0.7129

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have ran e2e tests and there is no issues.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2024 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@eunwoosh
Copy link
Contributor

So, purpose of this PR isn't to fix a bug but is for updating recipe to use same input size whlie train, val and test, right?

@eugene123tw
Copy link
Contributor Author

So, purpose of this PR isn't to fix a bug but is for updating recipe to use same input size whlie train, val and test, right?

yes, while I'm not entirely sure why the image size isn't aligned initially, this adjustment enhances the accuracy of tiling when using YOLOX-tiny. We've observed a 70% improvement on one of the Vitens datasets.

@sungchul2
Copy link
Contributor

So, purpose of this PR isn't to fix a bug but is for updating recipe to use same input size whlie train, val and test, right?

yes, while I'm not entirely sure why the image size isn't aligned initially, this adjustment enhances the accuracy of tiling when using YOLOX-tiny. We've observed a 70% improvement on one of the Vitens datasets.

Thanks for this update!
I also have no idea about this history for detection and tiling, but yolox-tiny (tiling or not) has different scales between training and val/test.
I'd like to know your comments about:

  1. exported model still uses (416, 416), do we change input size for the exported model?
    image_size = (1, 3, 416, 416)
    tile_image_size = (1, 3, 416, 416)
  2. do you think that it'd be good to change input size for detection? If so, could you check the performance difference briefly?
    input_size:
    - 416
    - 416

    input_size:
    - 416
    - 416

@eunwoosh
Copy link
Contributor

FYI, the reason why different input size is used it that mmdet uses like that. https://github.com/open-mmlab/mmdetection/blob/main/configs/yolox/yolox_tiny_8xb8-300e_coco.py

@sungchul2
Copy link
Contributor

FYI, the reason why different input size is used it that mmdet uses like that. https://github.com/open-mmlab/mmdetection/blob/main/configs/yolox/yolox_tiny_8xb8-300e_coco.py

Yes, and we don't need to stick with it if performance improves with the changed input size.

@eunwoosh
Copy link
Contributor

As Sungchul said, tile_image_size in YOLOXTINY class should be updated also. And could you update score table comparing between previous one and this change?

@eugene123tw
Copy link
Contributor Author

eugene123tw commented Jul 30, 2024

@eunwoosh @sungchul2 I ran some experiments comparing YOLOX-TINY with validation input sizes between 416 x 416 and 640 x 640. The accuracy differences are significant (>10%) on small object datasets (Vitens). However, for other regular object size datasets (PACKAGE, WGISD), there's not much difference.

Using a larger input size for training and a smaller one for validation seems reasonable for YOLOX-TINY if normal datasets (COCO, VOC, etc.) are used for training, as the model benefits from faster inference time due to the smaller input size. However, in my case, the latency difference is negligible, averaging around 10 ms. I don't mind updating the input size for both tiling and original input size to 640 x 640, as it seems like there's no harm in doing so—the difference in latency is small, and accuracy can be improved for small objects dataset.

Below are the results:

Model Val Input Size Dataset mAP
YOLOX-TINY 640 x 640 Vitens Coliform 0.4916
YOLOX-TINY 640 x 640 Vitens Aeromonas 0.8870
YOLOX-TINY 640 x 640 PACKAGE 0.8699
YOLOX-TINY 640 x 640 WGISD 0.6467
YOLOX-TINY 640 x 640 blueberry 0.8375
Model Val Input Size Dataset mAP
YOLOX-TINY 416 x 416 Vitens Coliform 0.3994
YOLOX-TINY 416 x 416 Vitens Aeromonas 0.7575
YOLOX-TINY 416 x 416 PACKAGE 0.8565
YOLOX-TINY 416 x 416 WGISD 0.6185
YOLOX-TINY 416 x 416 blueberry 0.7129
Model Val Input Size Dataset mAP
YOLOX-TINY-TILE 416 x 416 Vitens Kiemgetal 0.09
YOLOX-TINY-TILE 640 x 640 Vitens Kiemgetal 0.8645

@github-actions github-actions bot added the TEST Any changes in tests label Jul 30, 2024
@eunwoosh
Copy link
Contributor

I think updating both of them looks good to me. How do you think about it, @sungchul2 ?

@sungchul2
Copy link
Contributor

Actually, I think there is no reason not to change the input size in terms of significant performance improvements.
But I checked benchmark_app to compare throughput of both input sizes with benchmark_app -m exported_model.xml -nstream 1 -hint none -shape [1,3,416,416] or [1,3,640,640].
There is throughput degradation with (640, 640) and yolox-tiny showed slower performance than ssd_mobilenetv2 with default input size:

model input size throughput
yolox-tiny (1, 3, 416, 416) 135.66 FPS
yolox-tiny (1, 3, 640, 640) 69.31 FPS
atss_mobilenetv2 (1, 3, 800, 992) 58.78 FPS
ssd_mobilenetv2 (1, 3, 864, 864) 76.19 FPS

I think more discussion about this change is required because yolox-tiny is the 'speed' model in otx. What do you think?

@eunwoosh
Copy link
Contributor

It seems that it's ok to update yolox_tiny tiling model only for now.

@eugene123tw
Copy link
Contributor Author

@sungchul2 @eunwoosh I updated the files accordingly.

eunwoosh
eunwoosh previously approved these changes Jul 31, 2024
Copy link
Contributor

@sovrasov sovrasov left a comment

Choose a reason for hiding this comment

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

In general, LGTM, but is tile_image_size used on export?
What I see now is YOLOX._exporter uses self.image_size

kprokofi
kprokofi previously approved these changes Jul 31, 2024
@eugene123tw eugene123tw dismissed stale reviews from kprokofi and eunwoosh via e4a32eb July 31, 2024 16:54
@eugene123tw
Copy link
Contributor Author

In general, LGTM, but is tile_image_size used on export? What I see now is YOLOX._exporter uses self.image_size

Thanks, Vlad. Great catch! I forgot to update it in the exporter.

@eugene123tw eugene123tw added this pull request to the merge queue Aug 1, 2024
Merged via the queue into openvinotoolkit:develop with commit 106e425 Aug 1, 2024
18 checks passed
@eugene123tw eugene123tw deleted the eugene/improve-yolox-tiny-tile-recipe branch August 1, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TEST Any changes in tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants