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.

Sign up for GitHub

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

Jump to bottom

Adding Non-Maximum Suppression (NMS) utility to fiftyone.utils.labels through perform_nms() #4160

Merged
merged 4 commits into from
Mar 15, 2024
Merged

Adding Non-Maximum Suppression (NMS) utility to fiftyone.utils.labels through perform_nms() #4160

merged 4 commits into from
Mar 15, 2024

Conversation

rohis06-aws
Copy link
Contributor

@rohis06-aws rohis06-aws commented Mar 14, 2024 •
edited
Loading

What changes are proposed in this pull request?

Adds Non-Maximum Suppression (NMS) utility to fiftyone.utils.labels through perform_nms()! 🎉

It can be loaded as follows:

import fiftyone.utils.labels as fl

dataset = foz.load_zoo_dataset(
    "coco-2017",
    split="validation",
    dataset_name="detector-recipe",
)

predictions_view = dataset.take(10, seed=51)

fl.perform_nms(sample_collection=predictions_view, in_field="predictions", out_field="nms_predictions", conf_threshold=0.65, iou_threshold=0.4, progress=True)

How is this patch tested? If it is not, please explain why.

It has been thoroughly tested, as seen in this nms_test.pdf

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

Addition of perform_nms() utility to fiftyone.utils.labels

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

brimoor
brimoor requested changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

@rohis06 overall the method looks great, just have a few suggestions on implementation that would clean things up a bit if you're open to them! 😄

fiftyone/utils/labels.py Outdated
sample_collection: a
:class:`fiftyone.core.collections.SampleCollection`
in_field: the name of the :class:`fiftyone.core.labels.Detections` field
out_field: the name of the :class:`fiftyone.core.labels.Detections`
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making this optional: if not specified, the input field is updated in-place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that sounds like a good idea to me.
I'll make it optional.

fiftyone/utils/labels.py Outdated

with fou.ProgressBar(progress=progress) as pb:
for sample in pb(samples):
detections_data_json_str = sample[in_field].to_json()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you implement this by processing sample[in_field].detections as a list of Detection objects rather than serializing to JSON? We generally try to avoid converting to other formats if not necessary.

Note that you can call sample[in_field].copy() to get a copy of the detections so that the existing detections aren't modified in-place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, definitely! I will check out the .copy() method.

fiftyone/utils/labels.py Outdated
# Compare with other detections for NMS
for d in detections:
if d["label"] == selected_detection["label"]:
iou = _calculate_iou(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use the existing fiftyone.utils.iou.compute_ious() utility here to avoid re-implementing this calculation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll incorporate this. It's much better always to use an existing utility! 😄

@rohis06-aws
Copy link
Contributor Author

@rohis06 overall the method looks great, just have a few suggestions on implementation that would clean things up a bit if you're open to them! 😄

Thanks a lot for reviewing this, @brimoor. I really appreciate your feedback! 😄
I'm definitely open to suggestions, and I'll incorporate them to clean things up.

@rohis06-aws
Copy link
Contributor Author

@rohis06 overall the method looks great, just have a few suggestions on implementation that would clean things up a bit if you're open to them! 😄

Thanks a lot for reviewing this, @brimoor. I really appreciate your feedback! 😄 I'm definitely open to suggestions, and I'll incorporate them to clean things up.

@brimoor, I have addressed the review comments.
Could you let me know if this clears things up? 😄

@brimoor brimoor changed the base branch from develop to perform-nms March 15, 2024 22:07
brimoor
brimoor approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

@rohis06 thanks for your contribution! 🎉

@brimoor brimoor merged commit 1ace4e2 into voxel51:perform-nms Mar 15, 2024
6 of 7 checks passed
@rohis06-aws
Copy link
Contributor Author

@rohis06 thanks for your contribution! 🎉

Anytime! 😄

This was referenced Mar 16, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brimoor brimoor brimoor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rohis06-aws @brimoor