ultralytics icon indicating copy to clipboard operation
ultralytics copied to clipboard

Concat all segments by default for multi-part masks

Open Y-T-G opened this issue 1 year ago • 8 comments

Resolves https://github.com/ultralytics/ultralytics/issues/16733

If the mask of an object is split into multiple segments, ultralytics currently returns the largest segment during conversion from mask to segments. But most users would rather the segments in this scenario are concatenated.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Refactored the masks2segments function for better clarity and usability.

📊 Key Changes

  • Changed the masks2segments function parameter from strategy (string) to concat (boolean).
  • Updated code logic to reflect this change, simplifying conditional statements.

🎯 Purpose & Impact

  • Increased Clarity: The boolean parameter concat makes intended use clearer, enhancing usability for developers.
  • Simplification: Streamlines the code, making it easier to maintain and understand. This benefits both new and experienced users of the Ultralytics library.

Y-T-G avatar Oct 10 '24 14:10 Y-T-G

👋 Hello @Y-T-G, thank you for submitting a PR to the ultralytics/ultralytics repository! 🚀

To facilitate a smooth integration of your contribution, please ensure you have covered the following checklist:

  • Purpose Definition: Clearly articulate the purpose of your fix or feature in the PR description. Mention and link to any relevant issues you are addressing.
  • Stay Updated: Ensure your branch is up-to-date with the main branch. You can sync by selecting the 'Update branch' button or by running git pull and git merge main locally.
  • Pass CI Checks: Verify that all Continuous Integration (CI) checks are successful. If any checks fail, make sure to resolve them promptly.
  • Documentation Update: Reflect your changes in the relevant documentation to ensure users are well-informed of any new or adjusted features.
  • Testing: Add or update tests as needed to validate your changes. Confirm that all tests are passing.
  • Sign the CLA: If this is your first contribution, please sign our Contributor License Agreement by commenting “I have read the CLA Document and I sign the CLA.”
  • Minimal Changes: Aim to keep your changes as focused as possible on the specific bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential" — Bruce Lee

For further assistance, please refer to the Contributing Guide. If you have any questions, don't hesitate to ask. An Ultralytics engineer will be with you shortly to assist further. Thank you for contributing! 🌟

UltralyticsAssistant avatar Oct 10 '24 14:10 UltralyticsAssistant

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 74.86%. Comparing base (c5fd0bb) to head (f357c9e). Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16826      +/-   ##
==========================================
- Coverage   74.87%   74.86%   -0.01%     
==========================================
  Files         127      127              
  Lines       16845    16846       +1     
==========================================
  Hits        12612    12612              
- Misses       4233     4234       +1     
Flag Coverage Δ
Benchmarks 35.40% <25.00%> (-0.01%) :arrow_down:
GPU 31.62% <25.00%> (-0.01%) :arrow_down:
Tests 68.64% <100.00%> (-0.01%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

codecov[bot] avatar Oct 10 '24 14:10 codecov[bot]

Agree with this change, there were multiple issues in the past with the same issue and this behavior makes more sense.

Skillnoob avatar Oct 10 '24 17:10 Skillnoob

@Y-T-G Thanks for the PR! But in fact concatenating all the masks can result a wired mask like this https://github.com/ultralytics/ultralytics/issues/16733#issuecomment-2406365538 because all the coordinates might not be closed as a boundary of the object. And that's why we return the largest one, it might be a part of the whole object but the boundary is correct.

Laughing-q avatar Oct 11 '24 08:10 Laughing-q

@Laughing-q That's true. There's an inherent limitation with the format not being able to represent masks that are split up. I guess we should at least show a warning when this behavior occurs.

Y-T-G avatar Oct 11 '24 08:10 Y-T-G

@Laughing-q could create splits in segments by joining segment-groups with an float("nan") separator?

glenn-jocher avatar Oct 11 '24 16:10 glenn-jocher

@Laughing-q could create splits in segments by joining segment-groups with an float("nan") separator?

hmmm I tried but did not work to me as we need to scale coordinates to original image size domain and nan does not work with it, we could also add some logic to do this but it'll be a bit complicated.

@glenn-jocher @Y-T-G I think we can not satisfy every users cases and the largest option in fact works in most cases(in my opinion). Apparently there's way to get all the segments but need some customization on our code, which I would recommend we create and provide the customized code in our docs page. Then we can refer the docs to our users. What do you guys think? something like:

from ultralytics.utils import ops
model = YOLO("yolo11n-seg.pt")
for r in model.predict():
    segments = ops.masks2segments(r.masks.data, strategy="all")  # each segment is a list
    segments = [[ops.scale_coords(r.masks.shape[1:], x, r.orig_shape) for x in xy] for xy in segments]

We can deprecate concat since it usually returns distorted segments, and probably update it to all and return a list contains all the segments for each mask.

Laughing-q avatar Oct 14 '24 07:10 Laughing-q

@Laughing-q It can work I guess. Should be better than concat.

Y-T-G avatar Oct 14 '24 15:10 Y-T-G

Amazed this hasn't been addressed more thoroughly previously. Currently benchmarking YOLOv11 against another network and found very low segmentation performance, on closer inspection found that only the largest segmentations were returned. Pretty misleading to have result.masks only return a subset of the masks plotted using results.show()!

wsd3val avatar Nov 05 '24 12:11 wsd3val

@Y-T-G @glenn-jocher Hey guys! actually we have a function merge_multi_segment to merge all the separate segments into one, that I created years ago to merge multiple segments from COCO dataset into one before converting to yolo format. And it works to the COCO2YOLO conversion, perhaps we could directly implement it here. I've updated it in https://github.com/ultralytics/ultralytics/pull/16826/commits/c37c1a25807e086f852abeb89b62e1673372cc6a and tested it works to me. Can you test it as well? @Y-T-G THanks!

Laughing-q avatar Nov 08 '24 06:11 Laughing-q

@Laughing-q Seems to work pretty well from my tests.

Y-T-G avatar Nov 08 '24 08:11 Y-T-G

@Laughing-q PR merged! @Y-T-G thank you for the review and @wsd3val thank you for the feedback.

This should be live in 8.3.38 in the next 24 hours.

glenn-jocher avatar Nov 25 '24 15:11 glenn-jocher