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

Non-max-merging for Inference Slicer #1161

Closed
wants to merge 6 commits into from

Conversation

LinasKo
Copy link
Collaborator

@LinasKo LinasKo commented May 3, 2024

Description

This PR builds on top of @mario-dg PR #500, replacing it, fixing issues, adding non-max-merging to Detections and connecting to InferenceSlicer.

There main stylistic complexity here is dealing with individual objects inside Detections.

Here, the following changes are implemented:

  • Detections now have with_nmm, which merges the detections based on their overlap.
  • detections/core.py exposes an OverlapFiltering enum, which allows other methods to distinguish between the types. This enum is exposed globally in __init__.py.
  • InferenceSlicer is the only class using it right now. NMS is performed by default, and users may specify overlap_filtering to pick which filtering type to use.
  • New method Detections._set_at_index mimicks the former __setitem__, to be used internally. Only used in one location, so happy to refactor there.
  • Removed Optional from InferenceSlicer's iou_threshold arg.

⚠️ So far, only tested with object detections - not segmentation.
⚠️ Naming could be made more clear, e.g. in methods like utils.py::batch_non_max_merge. Ping me and I'll look into that.

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?

Tests:

Colab: https://colab.research.google.com/drive/1iiJ5oCl8dwwcefRgNXRdaFH6G9dVpf4P?usp=sharing

Any specific deployment considerations

One thing I couldn't figure out was the right way to access values of individual detected objects.
When a scalar was needed, I went with self.xyxy[i]. When an array was expected, I left the self[i].xyxy calls. Feels a bit backwards, but most concise - is there a better way?

Docs

  • Docs updated? What were the changes:

@LinasKo
Copy link
Collaborator Author

LinasKo commented May 3, 2024

@LinasKo
Copy link
Collaborator Author

LinasKo commented May 3, 2024

It fails when Detections.merge is called on segmentation detections. Frequently mask=None is returned, so detections in the Slicer can't be merged.

My guess is that it's caused by my old friend #943, but I have not verified.

@LinasKo
Copy link
Collaborator Author

LinasKo commented May 7, 2024

Closing; Moving fixes to #500 as discussed

@LinasKo LinasKo closed this May 7, 2024
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

1 participant