fractal-analytics-platform / fractal-tasks-core

Main tasks for the Fractal analytics platform
https://fractal-analytics-platform.github.io/fractal-tasks-core/
BSD 3-Clause "New" or "Revised" License
11 stars 5 forks source link

Fix wrong repeated overlap check for bounding-boxes in cellpose task #778

Closed tcompa closed 3 days ago

tcompa commented 3 days ago

Close #764

Checklist before merging

github-actions[bot] commented 3 days ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  fractal_tasks_core/tasks
  cellpose_segmentation.py
Project Total  

This report was generated by python-coverage-comment-action

tcompa commented 3 days ago

cc @lorenzocerrone or @jluethi This is ready from my side, and a second look would be welcome. I did not set up a specific benchmark (deferring this to #779), but it's anyway a fix that is needed (even if, by accident, it doesn't fix #764).

jluethi commented 3 days ago

Hmm, looking at this now: This (as well as the original implementation) will only check for potential bounding box overlaps within a given ROI that's being processed, right?

With the new implementation, we'd only check each ROI once, instead of checking all ROIs processed so far in every loop.

A future improvement may be to check for overlaps across ROIs as well, as in theory, they could also overlap. Given that this is just for warnings, not sure how relevant this is though. Our masking should still work for this after all.


Besides that: Looks good to me!

tcompa commented 3 days ago

Hmm, looking at this now: This (as well as the original implementation) will only check for potential bounding box overlaps within a given ROI that's being processed, right? With the new implementation, we'd only check each ROI once, instead of checking all ROIs processed so far in every loop.

I confirm. The PR is strictly equivalent to current behavior, but without the redundant calls.

tcompa commented 3 days ago

A future improvement may be to check for overlaps across ROIs as well, as in theory, they could also overlap.

Agreed, but let's do it in a future PR since it requires a bit of care to avoid bad scaling (ref https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/779#issuecomment-2208234941).