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

Fix possible image drawing issues #13423

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

zhixuwei
Copy link
Contributor

@zhixuwei zhixuwei commented Nov 21, 2024

Fixed non-meaningful labels in the top right (row 1, column 4) plots when batch size is larger than max_subplots.

Fixed non-meaningful labels in the top right (row 1, column 4) plots when batch size is larger than max_subplots.
The reason is that when the batch size is larger than max_subplots, the pre-repair plots will be looped one more time, and the unnecessary looping may accidentally draw the labels of the images indexed as max_subplots in the batch to the top-right corner of the image.
I fix the situation in this PR by using bs instead of i+1, plus I is not suitable for handling other logic as part of the for loop above.

I have read the CLA Document and I sign the CLA

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Improved the loop control in the image annotation function to prevent out-of-bounds errors.

📊 Key Changes

  • Adjusted the loop iteration condition in the plot_images() function to use min(bs, i + 1) instead of i + 1.

🎯 Purpose & Impact

  • Purpose: To ensure the loop does not exceed the number of available images, preventing potential errors during image annotation.
  • Impact: Enhances the robustness of the image plotting utility, reducing crashes and improving usability for large batches of images. 🖼️✨

Fixed non-meaningful labels in the top right (row 1, column 4) plots when batch size is larger than max_subplots.

Signed-off-by: zzzer <[email protected]>
@UltralyticsAssistant UltralyticsAssistant added bug Something isn't working fixed Bug has been resolved labels Nov 21, 2024
@UltralyticsAssistant
Copy link
Member

👋 Hello @zhixuwei, thank you for submitting an ultralytics/yolov5 🚀 PR! To ensure a seamless integration of your work, please review the following checklist:

  • Define a Purpose: You've explained the purpose beautifully! Remember to link any relevant issues if applicable.
  • Synchronize with Source: Ensure your PR is synchronized with the ultralytics/yolov5 main branch. Use the 'Update branch' button or run git pull and git merge main locally if it's behind.
  • Ensure CI Checks Pass: Verify all Ultralytics Continuous Integration (CI) checks are passing. If any checks fail, please address the issues.
  • Update Documentation: If any new or modified features affect usage, update the relevant documentation.
  • Add Tests: Include or update tests for your changes, ensuring all tests pass.
  • Sign the CLA: You've already signed the Contributor License Agreement, so no further action is needed here ✅.
  • Minimize Changes: Ensure your changes are as concise as possible to achieve the intended bug fix or feature enhancement.

Furthermore, for bug fixes, we request a Minimum Reproducible Example (MRE) if possible. If you haven't already provided it, this aids in understanding and verifying the fix.

For more guidance, please refer to our Contributing Guide. An Ultralytics engineer will review your PR shortly. Feel free to reach out with any questions or comments. Thanks again for your contribution! 🎉

@zhixuwei zhixuwei changed the title Update plots.py Fix possible image drawing issues Nov 21, 2024
@zhixuwei
Copy link
Contributor Author

train_batch2
The labels pointed by the yellow arrows are the labels in the batch data indexed by max_subplots. The current plot strategy ensures that the image labels are within the valid range of the image, so when the data indexed by the max_subplots in the batch exists the target, it will be shown in the top right corner of the image because of the robustness of the program.

@pderrenger
Copy link
Member

@zhixuwei thank you for your observation. It seems there might be an issue with how labels are being drawn when the batch size exceeds max_subplots. If you're experiencing any visual inconsistencies, you could try modifying the plotting logic to ensure labels are accurately placed. If the issue persists, please check if you're using the latest version of YOLOv5, as updates often include bug fixes and improvements. If the problem continues, feel free to open an issue on the YOLOv5 GitHub repository for further assistance. The community and the Ultralytics team are always here to help!

@zhixuwei
Copy link
Contributor Author

zhixuwei commented Nov 21, 2024

@zhixuwei thank you for your observation. It seems there might be an issue with how labels are being drawn when the batch size exceeds max_subplots. If you're experiencing any visual inconsistencies, you could try modifying the plotting logic to ensure labels are accurately placed. If the issue persists, please check if you're using the latest version of YOLOv5, as updates often include bug fixes and improvements. If the problem continues, feel free to open an issue on the YOLOv5 GitHub repository for further assistance. The community and the Ultralytics team are always here to help!

@pderrenger Most people don't notice this when training a model on a personal computer because the batch size is not set very large. I also stumbled upon this while tuning the training parameters. This was indeed a bug, so I submitted a PR to fix it.

@glenn-jocher glenn-jocher merged commit 81ac034 into ultralytics:master Nov 21, 2024
8 checks passed
@glenn-jocher
Copy link
Member

@zhixuwei PR merged!

@zhixuwei
Copy link
Contributor Author

@zhixuwei PR merged!

@glenn-jocher Thanks PR, but the correct number of draws can be satisfied by using bs as the loop condition. min(bs, i + 1) is not actually necessary, because in any case, bs is equal to min(bs, i+1). When batch size <= max_subplots, bs and i+1 have the same value, which means bs = i+1 = min(bs, i+1); When batch size > max_subplots, bs has the same value as i. bs = i = min(bs, i+1).

@glenn-jocher
Copy link
Member

Ah yes, you are correct! Can you open a second PR to fix this? Thank you!

@glenn-jocher
Copy link
Member

Can you test also with batch size argument larger than dataset size, i.e. with COCO8 and batch=16

zhixuwei added a commit to zhixuwei/yolov5 that referenced this pull request Nov 21, 2024
@zhixuwei
Copy link
Contributor Author

Ah yes, you are correct! Can you open a second PR to fix this? Thank you!

I resubmitted a PR in a concise manner

@zhixuwei
Copy link
Contributor Author

Can you test also with batch size argument larger than dataset size, i.e. with COCO8 and batch=16

@glenn-jocher I tested batch sizes of 5, 16, 17, and 32 on my own data set, and used bs as a loop condition.

@zhixuwei
Copy link
Contributor Author

zhixuwei commented Nov 23, 2024

@glenn-jocher
I've tested it on coco8 and it's no problem at all. This is due to the robustness of the data loader. The bs, as a loop condition, is not always the same as the value of the batch size set before training, but is the actual number of pictures in the batch.
Take coco8, batch_size=16 as an example, the data volume of the whole data set does not exceed 16, so in the data loading stage, the data volume of each batch is the full amount of training data, which does not affect the drawing, and there will be no index overstep problem.

The formula for bs is similar to: bs = min(batch_size, len(train_dataset))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Bug has been resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants