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

[Enhancement] Implement Distance-Based Detection Matching in DetectionMetrics #1463

Merged
merged 5 commits into from
Dec 8, 2023

Conversation

DimaBir
Copy link
Contributor

@DimaBir DimaBir commented Sep 13, 2023

Pull Request Description

This pull request introduces enhancements and additions related to distance-based metrics in the DetectionMetrics class. The main changes include:

  • Addition: The DetectionMetricsDistanceBased class has been added to support distance-based metrics for object detection.
    Abstract base class DetectionMatching, which defines the interface for matching implementations.
  • Modifications: The existing DetectionMetrics class has been extended to incorporate distance-based metrics when calculating precision, recall, F1, and mAP.

These changes improve the capabilities of the metrics calculation for object detection tasks, allowing users to analyze detection performance using distance-based criteria. Below are the details of the changes made:

Changes Made

  • Introduced the DetectionMetricsDistanceBased class, which extends the existing DetectionMetrics class to support distance-based metrics.
  • Modified the DetectionMetrics class to accept distance metric definitions and thresholds.
  • Updated the compute method in both classes to calculate metrics based on the results of distance-based matching when using DetectionMetricsDistanceBased.
  • Introduced an abstract base class, DetectionMatching, which defines the interface for matching implementations. It includes abstract methods for getting thresholds and computing metrics for targets and crowd targets.
  • Implemented the IoUMatching class to maintain backward compatibility with IoU-based metrics.
  • Added the DistanceMatching class for computing distance-based matching between predictions and targets.

Proposed Changes

  • The changes have been tested to ensure the accuracy and reliability of the distance-based metrics calculations.
  • The new classes and methods maintain compatibility with the existing functionality, ensuring backward compatibility.

Testing

  • Extensive testing has been performed to validate the accuracy and correctness of the metrics calculation.
  • The test cases cover various scenarios and edge cases for distance-based metrics.

This PR is ready for review and feedback. I would greatly appreciate your suggestions and suggestions for improvement.

@BloodAxe
Copy link
Contributor

Thanks for your PR. It looks like your PR contains commits that are not signed. We required all commits in the PR to be signed. You can use this suggestion to address that: #1465 (comment)

Maybe I'm missing something, but the whole idea of the PR is to allow using some other metric when matching pred vs gt boxes. So I have few questions regarding this:

  1. What are the use cases when one may want to use other metric?
  2. Can you provide any papers / project using custom bbox similarity metric?

I'm just not convinced this feature would be relevant for many users.

@DimaBir
Copy link
Contributor Author

DimaBir commented Sep 22, 2023

Thanks for your PR. It looks like your PR contains commits that are not signed. We required all commits in the PR to be signed. You can use this suggestion to address that: #1465 (comment)

Maybe I'm missing something, but the whole idea of the PR is to allow using some other metric when matching pred vs gt boxes. So I have few questions regarding this:

  1. What are the use cases when one may want to use other metric?
  2. Can you provide any papers / project using custom bbox similarity metric?

I'm just not convinced this feature would be relevant for many users.

Thank you for your feedback, and I apologize for the oversight regarding the signed commits. I will address this as soon as possible by signing all the commits in the PR according to the repository's guidelines. I will also review the suggestions in #1465 (comment) to ensure compliance.

Regarding your queries about the use of different metrics for bounding box similarity, here are my thoughts:

1. Use Cases for Different Metrics:

  • Distance-Based Metric:
    The task was to implement the distance-based metric to enhance existing Detection Metric to allow metrics based on distances from the bboxes' centers, particularly for small bboxes less than 5 pixels.
  • Custom Bbox Similarity Metrics in Research:
    The implementation of custom bbox similarity metrics is crucial, especially in the context of small object detection. Standard metrics like Intersection over Union (IoU) may not be suitable due to the specific challenges posed by small objects, necessitating the development of more tailored approaches.

2. References to Papers/Projects:

As an example of custom metrics we can look at article titled "A Comparative Analysis of Object Detection Metrics with a Companion Open-Source Toolkit" by Rafael Padilla, Wesley L. Passos, Thadeu L. B. Dias, Sergio L. Netto, and Eduardo A. B. da Silva, provides a comprehensive overview of the most relevant evaluation methods used in object detection competitions.

  • 7.1. Spatio-Temporal Tube Average Precision:
    The article proposes a new metric for evaluating video object detection, called the Spatio-Temporal Tube Average Precision. This metric is based on the spatio-temporal overlap between the ground truth and detected bounding boxes, emphasizing the importance of considering both spatial and temporal dimensions in object detection tasks, especially in video analysis.

3. Relevance of the Feature:

While the relevance of this feature might take time to become apparent, providing the flexibility to use different metrics can cater to a wider range of use cases and user needs.

DimaBir added a commit to DimaBir/super-gradients that referenced this pull request Sep 23, 2023
…ics (Deci-AI#1463)

- Introduced a new subclass, DetectionMetricsDistanceBased, to implement distance-based definitions of true positives, enhancing the existing DetectionMetrics class.
- Developed distance-based detection matching using Euclidean and Manhattan distance metrics, allowing for more versatile and accurate object detection evaluations.
- Added unit tests to ensure the reliability and accuracy of the new distance-based detection matching methodologies.
- Maintained backward compatibility by implementing IoUMatching and introduced DistanceMatching for new distance-based matching strategies.
- Refactored and cleaned up the code for better readability and compliance with coding standards, including applying Black for formatting.
@DimaBir DimaBir force-pushed the distance-based-metrics-refactored branch from 79e2d5c to 1487382 Compare September 23, 2023 08:07
@BloodAxe
Copy link
Contributor

@DimaBir I would like to proceed with this PR and get it merged. Can you please fix the merge conflict?

@DimaBir
Copy link
Contributor Author

DimaBir commented Nov 29, 2023

@DimaBir I would like to proceed with this PR and get it merged. Can you please fix the merge conflict?

Hi @BloodAxe, sure!
I will fix the conflicts ASAP until EoD.

@BloodAxe
Copy link
Contributor

Awesome, thanks!

@DimaBir
Copy link
Contributor Author

DimaBir commented Nov 30, 2023

Hi @BloodAxe, thanks for pointing this out. I will do np, tomorrow is it okay?

I have a few meetings to attend today 🫤

@BloodAxe
Copy link
Contributor

@DimaBir thanks for updating PR. Assuming build passes the overall PR looks good to me except a few inline comments I left. Nothing serious, mostly docs improvement.
However there is one blocker thing that needs to be addressed - this PR still contains unsigned commits and for that reason CI will not allow merging it. Fortunately the fix is pretty simple: https://github.com/Deci-AI/super-gradients/blob/master/CONTRIBUTING.md#i-have-created-pr-with-unsigned-commits-do-i-have-to-start-over

@BloodAxe
Copy link
Contributor

Hi @BloodAxe, thanks for pointing this out. I will do np, tomorrow is it okay?

I have a few meetings to attend today 🫤

Feel free to proceed with whatever pace is comfortable for you :) No rush here and thanks for your contribution!

@DimaBir
Copy link
Contributor Author

DimaBir commented Nov 30, 2023

Sure @BloodAxe I will fix everything and fix commits to signed.

@DimaBir DimaBir force-pushed the distance-based-metrics-refactored branch from 31ad623 to 3ed7359 Compare December 8, 2023 10:26
@DimaBir
Copy link
Contributor Author

DimaBir commented Dec 8, 2023

@DimaBir thanks for updating PR. Assuming build passes the overall PR looks good to me except a few inline comments I left. Nothing serious, mostly docs improvement. However there is one blocker thing that needs to be addressed - this PR still contains unsigned commits and for that reason CI will not allow merging it. Fortunately the fix is pretty simple: https://github.com/Deci-AI/super-gradients/blob/master/CONTRIBUTING.md#i-have-created-pr-with-unsigned-commits-do-i-have-to-start-over

Done, force pushed branch with signed commits

- Updated `compute_detection_matching` to conditionally require `iou_thresholds` only for IoU-based strategies.
Returned check if matching_strategy object is passed
- Updated `compute_img_detection_matching` to conditionally get thresholds using matching_strategy get_thresholds() method.

Note: Since thresholds passed via matching_strategy class, consider to remove iou_thresholds param
Copy link
Contributor

@BloodAxe BloodAxe left a comment

Choose a reason for hiding this comment

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

LGTM

@BloodAxe BloodAxe merged commit 953a671 into Deci-AI:master Dec 8, 2023
@DimaBir DimaBir deleted the distance-based-metrics-refactored branch December 22, 2023 11:11
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.

2 participants