Add Non-Maximum Merging (NMM) to Detections #500

mario-dg · 2023-10-18T22:30:42Z

Description

Currently it is only possible to apply Non-Maximum Suppression (NMS) to Detections.
In an attempt to increase the accuracy of object detection and segmentation, especially when using the InferenceSlicer,
I added the ability to apply NMM instead of NMS to the merged Detections merge_detections.

NMM merges bounding boxes and masks, that would have originally been discarded, when applying NMS.
This results in more accurate detections/segmentations.

The implementation of this algorithm closely follows the one from SAHI.

In addition, also inspired by SAHI, the InferenceSlicer can increase the detection accuracy of larger objects, by running inference
on the whole image on top of all slices perform_standard_pred. The whole image detections will be merged with the sliced image detections.

I would be happy to discuss implementation changes and design choices.
This PR should serve as a first implementation and is far from perfect.
Any ideas, critic or comments are more than welcome :)

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How has this change been tested, please provide a testcase or example of how you tested the change?

NMS:

NMM:

SAHI:

The code sample that was used to create these two images:

import cv2
import numpy as np
import supervision as sv

from supervision.detection.core import Detections
from supervision.detection.tools.inference_slicer import InferenceSlicer

from PIL import Image
from ultralytics.models.yolo import YOLO


model = YOLO("yolov8s.pt")
def callback(x):
    result = model(x[:, :, ::-1])[0]
    dets = Detections.from_ultralytics(result)
    return dets[dets.confidence > 0.3]

# get image from here: https://ultralytics.com/images/bus.jpg

slicer = InferenceSlicer(callback=callback, 
                         slice_wh=(512, 512), 
                         overlap_ratio_wh=(0.4, 0.4), 
                         iou_threshold=0.1, 
                         merge_detections=True, 
                         perform_standard_pred=True)

ba = sv.BoundingBoxAnnotator()
la = sv.LabelAnnotator()
image = np.array(Image.open("bus.jpg"))

nmm_dets = slicer(image)
nmm_ann = ba.annotate(scene=image.copy(), detections=nmm_dets)
nmm_ann = la.annotate(scene=nmm_ann, detections=nmm_dets)
cv2.imwrite("nmm.jpg", nmm_ann)

slicer.merge_detections = False

nms_dets = slicer(image)
nms_ann = ba.annotate(scene=image.copy(), detections=nms_dets)
nms_ann = la.annotate(scene=nms_ann, detections=nms_dets)
cv2.imwrite("bms.jpg", nms_ann)

Any specific deployment considerations

The two additional parameters that were added to InferenceSlicer are optional and default to False, thus neither breaking, nor changing the results of existing implementations.

Docs

Docs updated? What were the changes: I will update the docs in the near future.

…merging

…e large object detection accuracy

…rrays

SkalskiP · 2023-10-20T14:09:26Z

Hi @mario-dg 👋🏻! Sorry for lagging with the review. I will take a look at it on Monday.

mario-dg · 2023-10-20T16:43:07Z

Hi @SkalskiP👋, no worries!
Must have been quite busy with the latest Release! Congrats on that one🚀

kadirnar · 2024-01-07T15:54:09Z

This is great feature. When will it be merged? @SkalskiP

SkalskiP · 2024-01-07T16:39:20Z

@kadirnar, I need to review it first. I didn't have time to work on it yet.

SkalskiP · 2024-04-08T12:57:22Z

Hi @mario-dg 👋🏻 We aim to significantly enhance our InferenceSlicer implementation. One of the challenges we've encountered involves limitations associated with applying non_max_suppression in the area of overlapping tiles. Would you still be interested in continuing to assist us?

mario-dg · 2024-04-09T06:41:11Z

Hey @SkalskiP, thanks for coming back to this PR!
Sure, I would love to continue🚀

SkalskiP · 2024-04-09T07:24:47Z

Hi @mario-dg 👋🏻 that's awesome!

We are planning a major wave of updates in InferenceSlicer that includes:

Support for segmentation models
Support for batch processing (running inference on multiple tiles at once)
Improved post-processing - merging instead of removing overlapping detections

This PR looks like the perfect candidate to achieve the last of these goals.

mario-dg · 2024-04-09T07:29:59Z

Thats great! Supervision might become a much more suitable lightweight alternative to SAHI, once we've improved its capabilities. Anything specific I could work on?

SkalskiP · 2024-04-09T08:24:03Z

@mario-dg 🔥 Please start by solving conflicts. This PR is quite old and we need to make it up to date. Than @LinasKo will review the PR.

LinasKo · 2024-04-09T08:25:11Z

Hi @mario-dg 👋

We spoke with SkalskiP - I'll see if I can review your code today.
We know of a few changes still needed, but I need to ramp-up a bit.

The broader context is that there's another candidate solution #268 and we'd like to compare the two before adding to InferenceSlicer.

mario-dg · 2024-04-09T09:54:55Z

Alright @SkalskiP, will get to it this evening.

LinasKo

Hey @mario-dg,

I've reviewed the PR. Frankly, I think it's a really well structured, well-documented contribution. I especially like how you managed to match the function API and documentation to what we have in with_nms 🙂

I've noted down the changes that we'd need. Some extra tests would be good to do as well.

You can find one here: https://storage.googleapis.com/com-roboflow-marketing/supervision/video-examples/beach-1.mp4
There's some code to run the models here. We're especially interested in the case of from_inference as that may produce the data object I mentioned in a few comments. https://supervision.roboflow.com/develop/how_to/detect_small_objects/

Let me know if you have any questions.

Hey @SkalskiP, as mentioned in one of the comments - you were right about data missing in __setitem__.

The broader question is: what's contained inside it? Do we have a list of possible dict entries? We would want to unify into a single object, so it's a different 'merge' than what we had before in Detections.merge.

supervision/detection/core.py

supervision/detection/utils.py

supervision/detection/core.py

supervision/detection/tools/inference_slicer.py

mario-dg · 2024-04-09T13:54:21Z

Thank you @LinasKo for this in depth review. I think I got most of what you've commented.
I estimate the changes you've requested to be done this work week.

SkalskiP · 2024-04-10T06:36:48Z

Hey @SkalskiP, as mentioned in one of the comments - you were right about data missing in __setitem__.

The broader question is: what's contained inside it? Do we have a list of possible dict entries? We would want to unify into a single object, so it's a different 'merge' than what we had before in Detections.merge.

The whole point of the data field is to give people flexibility. The data can contain anything. It seems to me that the most challenging task will involve linking two or more objects together. At the moment, I'm leaning towards copying the data from the detection with the higher confidence value. @LinasKo, what do you think?

LinasKo · 2024-04-10T06:48:59Z

The whole point of the data field is to give people flexibility. The data can contain anything. It seems to me that the most challenging task will involve linking two or more objects together. At the moment, I'm leaning towards copying the data from the detection with the higher confidence value. @LinasKo, what do you think?

Got it; then there's no ideal solution. In light of that - more confidence is a nice tie-breaker.

@mario-dg, can I request a change? If we're relying on confidence as a tie-breaker, let's make the class_id and tracker_id resolution use the same logic - instead of using min and max, let's pick the IDs from the more confident of the two merged objects.

mario-dg · 2024-04-10T08:27:02Z

@LinasKo yes sure! Was about to start with your first comments, so I will keep that in mind and use the confidences as a base to determine which ids will be picked in the merging process.

mario-dg · 2024-04-11T10:24:30Z

@SkalskiP, @LinasKo I have just pushed my latest changes and would love for you to look over them again!
I'm stilling missing tests though. Will get to them as soon as I can🔥

LinasKo · 2024-04-11T18:24:57Z

Hey @mario-dg,

Good work! I'm reviewing it now.

For testing, you might find something useful in this Colab. If you remove the inference slicer and its callbacks, you should be able to evaluate the runtime of your functions on an image with many realistic detections.

If you do use it, I suggest you remove surplus tests - Ultralytics runs quickest and is easy to install. Also, I don't think you'll need roboflow, inference-gpu, transformers, and timm packages.

LinasKo · 2024-04-11T19:02:17Z

Other than that, I like how this looks.
It would be cool to see how fast it runs :)

LinasKo · 2024-05-03T07:43:12Z

Upon closer inspection, mypy revealed issues relating to datatypes.

Specifically, many of the variables we have such as class_id and tracker_id may be None. Also, in the output we expect numpy arrays, whereas the code returns floats and a list.

We'd like to merge today, so I'll see if I can fix it.

mario-dg · 2024-05-03T08:43:43Z

Hey @LinasKo, sorry for being absent lately. Due to being pretty busy with work and my masters thesis, I'm currently not able to test or benchmark.

LinasKo · 2024-05-03T08:54:39Z

It's alright; we've all been through that 😉

Thank you very much for your contribution. I'll make sure it gets verified and merged soon!

LinasKo · 2024-05-15T07:04:12Z

Hey @SkalskiP, I pushed an update; here's some questions:

Removed non_max_merge altogether - it wasn't used.
There's now tests for merge_object_detection_pair and box_non_max_merge, but not for batch_box_non_max_merge. Would you like to see those too?
The merge_object_detection_pair I kept in detections/core.py. Due to it using Detections, it'd be a circular dependency if moved to utils. Alternatively, It would work if I moved it to utils, but annotate with "Detections" type without importing Detections from core.
with_nmm is between 2x and 10x slower than with_nms.
- For 1000 iterations & 38 detections, it's 10x slower. (0.42s vs 4.7s total)
- For 1000 iterations & 758 detections, it's 2x slower. (41s vs 95s)
- For 1 iteration & 38 detections: 5x slower (1ms vs 5ms)
- For 1 iteration & 758 detections: 1.5x slower (42ms vs 89ms)
I wanted to check with you again regarding datatypes. Do we want the change to these functions or every detections/utils method? that's 117 changes.
box_non_max_merge tie-breaks preferring later detections. E.g. given equal confidence, and sufficient overlap, we would merge detections 0 and 1 into detection 2. I think it's too small to care about - do we agree on that?

* Ruff complains when `== True` is used * Different behaviour with `is True`

SkalskiP · 2024-05-15T18:29:08Z

Removed non_max_merge altogether - it wasn't used.

Awesome! Less is more!

There's now tests for merge_object_detection_pair and box_non_max_merge, but not for batch_box_non_max_merge. Would you like to see those too?

No. That's okey.

The merge_object_detection_pair I kept in detections/core.py. Due to it using Detections, it'd be a circular dependency if moved to utils. Alternatively, It would work if I moved it to utils, but annotate with "Detections" type without importing Detections from core.

Yup. Let's move it and use "Detections". It's not perfect but we need to keep it simple in detections/core.py.

I wanted to check with you again regarding datatypes.

I'm really sorry but I'm not sure what you are asking. Sorry once again.

LinasKo · 2024-05-16T07:29:24Z

Thanks, that's helpful :)

I've now realized that for the method we planned to move to utils, annotating with "Detections". Is not enough. It needs to know about the Detections class as it calls the constructor.
- I strongly suggest we keep it in core.py. Otherwise it's either a circular import, or we need to pass the class as an argument which adds complication.
- Is there a third, non-complex option?
Indeed, I was unclear when I asked about datatypes. What I meant was npt.NDArray[np.float64]. We can change the type annotations only in code introduced by this PR, or we can change it in the whole core.py and utils.py files. Alternatively, we can wait for @onuralpszr to push his changes (Issue 1021), and then do a second pass, annotating every np.ndarray that's left (We'll need to do that anyway - some changes will have been made on other branches).

LinasKo · 2024-05-17T08:01:06Z

Added np.NDArray types for functions affected by this change.

I believe I've addressed the change requests.

supervision/__init__.py

SkalskiP · 2024-05-21T14:51:42Z

supervision/detection/core.py

+        return Detections.merge(result)
+
+
+def merge_object_detection_pair(det1: Detections, det2: Detections) -> Detections:


Please rename the arguments to detections_1 and detections_2.

In general, I think we have a naming problem. Our current marge should be called concatenate, and this should be just merge. But as long as merge_object_detection_pair is not part of public API we don't need to overthink it.

I made an attempt to improve the naming:

box_non_max_merge -> _box_non_max_merge_all (open to name ideas)

box_non_max_merge_batch -> box_non_max_merge. Now it's the main method, exactly like * box_non_max_suppression

merge_object_detection_pair -> _merge_inner_detection_object_pair

new method: _merge_inner_detections_objects

SkalskiP · 2024-05-21T15:03:35Z

supervision/detection/core.py

@@ -1066,6 +1068,33 @@ def __setitem__(self, key: str, value: Union[np.ndarray, List]):

        self.data[key] = value

+    def _set_at_index(self, index: int, other: Detections):


Wouldn't placing this code as part of the setitem method makes more sense? The flow below feels quite natural to me.

detections_1 = sv.Detections(...) detections_2 = sv.Detections(...) detections_1[0] = detections_2[0]

__setitam__

detections_2[0] detections_2["class_name"]

__getitam__

detections_2[0] detections_2[1:3] detections_2[[1, 2, 3]] detections_2[[False, True, False]] detections_2["class_name"]

_set_at_index was not required. I removed it entirely, and did not add any logic to __setitem__.

supervision/detection/core.py

SkalskiP · 2024-05-21T15:22:07Z

supervision/detection/utils.py

@@ -274,6 +275,81 @@ def box_non_max_suppression(
    return keep[sort_index.argsort()]


+def box_non_max_merge(


This one seems like a helper function that does not need to be exposed in the API.

renamed to _box_non_max_merge_all, removed from init.py.

Note: you'll see box_non_max_merge which is the prior batch function. Now it's the main one.

supervision/detection/core.py

supervision/__init__.py

supervision/detection/utils.py

* Reintroduced iou check before response - necessary for algorithm

LinasKo · 2024-05-23T13:45:33Z

@SkalskiP, Here's some updates. I've addressed every comment you had, with the major changes being as follows:

Removed _set_at_index, without any logic changes to __setitem__ - I found that we don't need it.
I made an attempt to improve the naming:
- box_non_max_merge -> _box_non_max_merge_all (open to name ideas)
- box_non_max_merge_batch -> box_non_max_merge. Now it's the main method, exactly like box_non_max_suppression
- merge_object_detection_pair -> _merge_inner_detection_object_pair
- new method: _merge_inner_detections_objects
Simplified the with_nmm function, albeit at the cost of 2 helper methods. Now it's similar to with_nms.
Modified the methods to use List[List[int]] as the core datatype. It does look simpler now.

Unexpected changes:

I found that doing the iou check during the final merging was important. In rare cases, it would prevent unintended merges. Let's chat if you want a deeper explanation.

Same colab as before, verified to work as the original author intended: https://colab.research.google.com/drive/1v0MPlG1tQctX5-koh0l6h1NcB6eWJ_YY#scrollTo=hObeS7Dg8_AP

Let me know what you think.

supervision/detection/utils.py

supervision/detection/core.py

SkalskiP · 2024-05-27T11:02:41Z

supervision/detection/core.py

+    Raises:
+        ValueError: If one field is None and the other is not, for any of the fields.
+    """
+    attributes = ["mask", "confidence", "class_id", "tracker_id"]


Can we try to get that list automatically?

I tried it, it's cumbersome, I'll add the code + tests in a separate PR and we can choose whether to keep it.

supervision/detection/core.py

SkalskiP · 2024-05-27T11:19:05Z

supervision/detection/core.py

+
+        result = []
+        for merge_group in merge_groups:
+            unmerged_detections = [self[i] for i in merge_group]


Maybe we don't need that list comprehension, just use detections[indexes].

My explanation was wrong.

We're doing this not to copy the result (that's in another case), but to create a list of single-object detections. [Detections, Detections, Detections, ...].

I believe this is the most concise way.

LinasKo · 2024-05-27T13:26:05Z

Hi @SkalskiP,

I've tidied this up - I believe it can now be merged.

SkalskiP · 2024-05-27T19:35:17Z

Thanks a lot, @LinasKo and @mario-dg! 🙏🏻 It's merged!

mario-dg · 2024-05-27T20:41:54Z

That's awesome! Didn't image that this PR would turn out as big as it got. Thanks guys!🚀

mario-dg added 4 commits October 13, 2023 18:24

feat: 🚀 Added Non-Maximum Merging to Detections

c78ae33

Added __setitem__ to Detections and refactored the object prediction …

57b12e6

…merging

Added standard full image inference after sliced inference to increas…

9f22273

…e large object detection accuracy

Refactored merging of Detection attributes to better work with np.nda…

6f47046

…rrays

SkalskiP mentioned this pull request Apr 9, 2024

Add polygon rotation functions #1098

Closed

1 task

LinasKo mentioned this pull request Apr 9, 2024

[weighted_box_fussion] - an alternative for box_non_max_suppression #268

Open

2 tasks

LinasKo requested changes Apr 9, 2024

View reviewed changes

Merge branch 'develop' into add_nmm_to_detections to resolve conflicts

5f0dcc2

Implement Feedback

166a8da

fix: merge_object_detection_pair

2d740bd

Linas Kondrackis added 5 commits May 15, 2024 10:46

Rename to batch_box_non_max_merge to box_non_max_merge_batch

145b5fe

box_non_max_merge: use our functions to compute iou

6c40935

Minor renaming

53f345e

Revert np.bool comparisons with is

0e2eec0

* Ruff complains when `== True` is used * Different behaviour with `is True`

Simplify box_non_max_merge

559ef90

LinasKo force-pushed the add_nmm_to_detections branch from fe11936 to 559ef90 Compare May 15, 2024 09:14

Removed suprplus NMM code for 20% speedup

f8f3647

Add npt.NDarray[x] types, remove resolution_wh default val

9024396

SkalskiP requested changes May 21, 2024

View reviewed changes

supervision/detection/core.py Outdated Show resolved Hide resolved

supervision/__init__.py Show resolved Hide resolved

supervision/detection/utils.py Outdated Show resolved Hide resolved

supervision/detection/utils.py Outdated Show resolved Hide resolved

Linas Kondrackis and others added 3 commits May 23, 2024 16:01

Address review comments, simplify merge

6fbca83

* Reintroduced iou check before response - necessary for algorithm

fix(pre_commit): 🎨 auto format pre-commit hooks

db1b473

Remove _set_at_index

0721bc2

LinasKo mentioned this pull request May 24, 2024

Prediction-level metadata in sv.Detections #1226

Closed

2 tasks

SkalskiP reviewed May 27, 2024

View reviewed changes

Linas Kondrackis added 2 commits May 27, 2024 16:17

Address comments

530e1d0

Renamed to group_overlapping_boxes

2ee9e08

SkalskiP approved these changes May 27, 2024

View reviewed changes

SkalskiP merged commit a0d0d45 into roboflow:develop May 27, 2024
9 checks passed

mario-dg deleted the add_nmm_to_detections branch May 28, 2024 05:24

SkalskiP mentioned this pull request Jun 5, 2024

supervision-0.21.0 release #1258

Merged

		return Detections.merge(result)


		def merge_object_detection_pair(det1: Detections, det2: Detections) -> Detections:

		@@ -1066,6 +1068,33 @@ def __setitem__(self, key: str, value: Union[np.ndarray, List]):

		self.data[key] = value

		def _set_at_index(self, index: int, other: Detections):

		@@ -274,6 +275,81 @@ def box_non_max_suppression(
		return keep[sort_index.argsort()]


		def box_non_max_merge(

Add Non-Maximum Merging (NMM) to Detections #500

Add Non-Maximum Merging (NMM) to Detections #500

Conversation

mario-dg commented Oct 18, 2023 • edited Loading

Description

Type of change

How has this change been tested, please provide a testcase or example of how you tested the change?

Any specific deployment considerations

Docs

SkalskiP commented Oct 20, 2023

mario-dg commented Oct 20, 2023

kadirnar commented Jan 7, 2024

SkalskiP commented Jan 7, 2024

SkalskiP commented Apr 8, 2024

mario-dg commented Apr 9, 2024

SkalskiP commented Apr 9, 2024

mario-dg commented Apr 9, 2024

SkalskiP commented Apr 9, 2024

LinasKo commented Apr 9, 2024

mario-dg commented Apr 9, 2024

LinasKo left a comment • edited Loading

Choose a reason for hiding this comment

mario-dg commented Apr 9, 2024

SkalskiP commented Apr 10, 2024

LinasKo commented Apr 10, 2024

mario-dg commented Apr 10, 2024

mario-dg commented Apr 11, 2024

LinasKo commented Apr 11, 2024 • edited Loading

LinasKo commented Apr 11, 2024 • edited Loading

LinasKo commented May 3, 2024 • edited Loading

mario-dg commented May 3, 2024

LinasKo commented May 3, 2024

LinasKo commented May 15, 2024 • edited Loading

SkalskiP commented May 15, 2024 • edited Loading

LinasKo commented May 16, 2024

LinasKo commented May 17, 2024

Choose a reason for hiding this comment

SkalskiP May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinasKo May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinasKo commented May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinasKo commented May 27, 2024

SkalskiP commented May 27, 2024

mario-dg commented May 27, 2024

mario-dg commented Oct 18, 2023 •

edited

Loading

LinasKo left a comment •

edited

Loading

LinasKo commented Apr 11, 2024 •

edited

Loading

LinasKo commented Apr 11, 2024 •

edited

Loading

LinasKo commented May 3, 2024 •

edited

Loading

LinasKo commented May 15, 2024 •

edited

Loading

SkalskiP commented May 15, 2024 •

edited

Loading

SkalskiP May 21, 2024 •

edited

Loading

LinasKo May 23, 2024 •

edited

Loading

LinasKo commented May 23, 2024 •

edited

Loading