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

Parameterize max_det + inference default at 1000 #3215

Merged
merged 6 commits into from
May 17, 2021
Merged

Parameterize max_det + inference default at 1000 #3215

merged 6 commits into from
May 17, 2021

Conversation

adrianholovaty
Copy link
Contributor

@adrianholovaty adrianholovaty commented May 17, 2021

In this pull request, I've changed max_det (the maximum number of detections per image) so that it's more easily configurable. Previously it was always hard-coded to 300, and the only way to change it was to edit the Python code.

Everything is backwards compatible — i.e., the default hasn't changed. Here's what's different:

  • utils.general.non_max_supression() now takes a max_det parameter.
  • detect.py now has a --max-det command-line parameter, which lets you set the value.
  • The NMS and AutoShape classes in models/common.py allow overriding max_det by setting the attribute on the class.

I'm not sure whether the third thing (the models/common.py changes) is actually useful, as I don't know how that part of the code works. I sort of guessed based on the fact that the two classes already had some similar attributes (conf, iou, classes).

Finally, I should say it felt a bit dirty to copy the default max_det=300 so that it's now duplicated in four separate places. I'd suggest putting that 300 value in a single constant somewhere, then change all four places to use the constant instead of hard-coding 300. That way the value would only live in a single place. I'm happy to do this work; just let me know if there's an existing place for constants or whether I should create one.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Adjustment to the max number of detections in YOLOv5's NMS process.

📊 Key Changes

  • The max_det parameter (maximum detections per image) has been introduced to non_max_suppression function.
  • Default max_det value has been set to 1000, up from the previous hard-coded value of 300.
  • The detect.py, common.py, and general.py files are updated to pass and use the max_det parameter.

🎯 Purpose & Impact

  • Purpose: To allow configuration of the maximum number of detections, facilitating balance between performance and resource constraints for different use cases.
  • Impact: Users now can detect more objects per image, which is particularly useful for high-resolution images or scenarios with numerous small objects. This may marginally impact the processing time and resources used during detection.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

👋 Hello @adrianholovaty, thank you for submitting a 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify your PR is up-to-date with origin/master. If your PR is behind origin/master an automatic GitHub actions rebase may be attempted by including the /rebase command in a comment body, or by running the following code, replacing 'feature' with the name of your local branch:
git remote add upstream https://github.com/ultralytics/yolov5.git
git fetch upstream
git checkout feature  # <----- replace 'feature' with local branch name
git rebase upstream/master
git push -u origin -f
  • ✅ Verify all Continuous Integration (CI) checks are passing.
  • ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential. The closer to the source, the less wastage there is." -Bruce Lee

@glenn-jocher glenn-jocher changed the title Parameterize max_det in various places Parameterize max_det + inference default at 1000 May 17, 2021
@glenn-jocher
Copy link
Member

@adrianholovaty looks good, thanks for the PR. I've updated it for PEP8 and I've increased the inference default max_det value to 1000 boxes for PyTorch Hub and detect.py, with test.py remaining at 300 to not slow down testing.

@glenn-jocher
Copy link
Member

@adrianholovaty BTW I've increased the default as we've had a few issues/questions raised about this given the current 300 value default. Hopefully this will reduce instances of hitting the ceiling. For most common use cases (i.e. zidane.jpg and bus.jpg at --conf 0.25) the performance impact will be zero.

@glenn-jocher glenn-jocher merged commit 3f74cd9 into ultralytics:master May 17, 2021
@glenn-jocher
Copy link
Member

@adrianholovaty PR is merged. Thank you for your contributions!

@adrianholovaty
Copy link
Contributor Author

@glenn-jocher Thanks! Glad to contribute back to this great project. :)

@VdLMV
Copy link
Contributor

VdLMV commented May 23, 2021

@glenn-jocher If you increase max_det, is it then not also a good idea to increase max_nms?
Maybe something like:
max_nms = max_det * 100

Maybe there also a need for increasing time_limit?

@glenn-jocher
Copy link
Member

glenn-jocher commented May 23, 2021

@VdLMV no it's not necessary, and in fact the use case for large numbers of boxes into the torch NMS function has an unchanged max_det value of 300.

Lechtr pushed a commit to Lechtr/yolov5 that referenced this pull request Jul 20, 2021
* Added max_det parameters in various places

* 120 character line

* PEP8

* 120 character line

* Update inference default to 1000 instances

* Update inference default to 1000 instances

Co-authored-by: Glenn Jocher <glenn.jocher@ultralytics.com>
(cherry picked from commit 3f74cd9)
BjarneKuehl pushed a commit to fhkiel-mlaip/yolov5 that referenced this pull request Aug 26, 2022
* Added max_det parameters in various places

* 120 character line

* PEP8

* 120 character line

* Update inference default to 1000 instances

* Update inference default to 1000 instances

Co-authored-by: Glenn Jocher <glenn.jocher@ultralytics.com>
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.

3 participants