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 confidence threshold for ClearML debug images #9174

Merged
merged 3 commits into from
Aug 26, 2022

Conversation

HighMans
Copy link
Contributor

@HighMans HighMans commented Aug 26, 2022

The confidence is converted to a percentage on line 144, but it is being compared to a default conf_threshold value of a decimal value instead of percent value.

Signed-off-by: HighMans [email protected]

πŸ› οΈ PR Summary

Made with ❀️ by Ultralytics Actions

🌟 Summary

Improved variable naming and fixed confidence threshold logic in ClearML image logging.

πŸ“Š Key Changes

  • Renamed the confidence variable to confidence_percentage for better clarity.
  • Corrected the threshold comparison to use the raw confidence value (conf) instead of the percentage.

🎯 Purpose & Impact

  • πŸ‘ Enhances code readability by making variable names more descriptive.
  • 🐞 Fixes an issue where the confidence percentage was incorrectly used in the threshold comparison, ensuring only relevant detections are logged. This can lead to more accurate debugging and visualization in ClearML.

The confidence is converted to a percentage on line 144, but it is being compared to a default conf_threshold value of a decimal value instead of percent value.

Signed-off-by: HighMans <[email protected]>
@HighMans HighMans changed the title Fix confidence threshold Fix confidence threshold for ClearML debug images Aug 26, 2022
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 @HighMans, thank you for submitting a YOLOv5 πŸš€ PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • βœ… Verify your PR is up-to-date with upstream/master. If your PR is behind upstream/master an automatic GitHub Actions merge may be attempted by writing /rebase in a new comment, 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 merge 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

@AyushExel
Copy link
Contributor

Thanks for the PR!
I think this makes sense as the confidence is being compared after being multiplied by 100. Right @thepycoder
?

confidence = round(float(conf) * 100, 2)

@HighMans
Copy link
Contributor Author

Thanks for the PR! I think this makes sense as the confidence is being compared after being multiplied by 100. Right @thepycoder ?

confidence = round(float(conf) * 100, 2)

The confidence is converted to a percentage here:

confidence = round(float(conf) * 100, 2)

But then it's being compared two lines after here to conf_threshold which by default is .25 and not 25%:

if confidence > conf_threshold:

def log_image_with_boxes(self, image_path, boxes, class_names, image, conf_threshold=0.25):

@AyushExel
Copy link
Contributor

Yup. Makes sense

@thepycoder
Copy link
Contributor

thepycoder commented Aug 26, 2022

Hey nice catch! That was sloppy coding from my part.

That said, don't you think it makes more sense to convert the percentage to decimal and keep the threshold like it is? I think this is pretty much default when passing along a threshold right? That it is between 0 and 1?

If you agree, can you leave the argument as it is, but just set the confidence to a decimal instead of a percentage? Then inside the f-string we can multiply it by 100 for visualisation, but it would have no effect on the original parameter which can then be properly compared to the threshold :)

@AyushExel
Copy link
Contributor

@thepycoder yes I think it makes most sense to just keep the decimal and not convert it to percentage.

Fix the confidence percentage is being compared to a decimal value.
@HighMans
Copy link
Contributor Author

HighMans commented Aug 26, 2022

That makes a lot more sense, should be fixed now.

class_name = class_names[int(class_nr)]
confidence_percentage = round(float(conf) * 100, 2)
label = f"{class_name}: {confidence_percentage}%"
if conf > conf_threshold:
annotator.rectangle(box.cpu().numpy(), outline=color)
annotator.box_label(box.cpu().numpy(), label=label, color=color)

@thepycoder
Copy link
Contributor

That makes a lot more sense, should be fixed now.

class_name = class_names[int(class_nr)]
confidence_percentage = round(float(conf) * 100, 2)
label = f"{class_name}: {confidence_percentage}%"
if conf > conf_threshold:
annotator.rectangle(box.cpu().numpy(), outline=color)
annotator.box_label(box.cpu().numpy(), label=label, color=color)

Lgtm! Thank a lot :)

@glenn-jocher glenn-jocher merged commit ffbce38 into ultralytics:master Aug 26, 2022
@glenn-jocher
Copy link
Member

@HighMans @thepycoder @AyushExel PR is merged. Thank you for your contributions to YOLOv5 πŸš€ and Vision AI ⭐

ctjanuhowski pushed a commit to ctjanuhowski/yolov5 that referenced this pull request Sep 8, 2022
* Fix confidence threshold

The confidence is converted to a percentage on line 144, but it is being compared to a default conf_threshold value of a decimal value instead of percent value.

Signed-off-by: HighMans <[email protected]>

* Revert "Fix confidence threshold"

This reverts commit f84a099.

* Fix confidence comparison

Fix the confidence percentage is being compared to a decimal value.

Signed-off-by: HighMans <[email protected]>
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.

None yet

4 participants