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

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

Merged

Conversation

tcompa
Copy link
Collaborator

@tcompa tcompa commented Jul 3, 2024

Close #764

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

@tcompa tcompa linked an issue Jul 3, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Jul 3, 2024

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
Copy link
Collaborator Author

tcompa commented Jul 3, 2024

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
Copy link
Collaborator

jluethi commented Jul 3, 2024

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
Copy link
Collaborator Author

tcompa commented Jul 4, 2024

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
Copy link
Collaborator Author

tcompa commented Jul 4, 2024

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 #779 (comment)).

@tcompa tcompa merged commit f306ab5 into main Jul 4, 2024
@tcompa tcompa deleted the 764-cellpose-task-output-roi-table-creation-performance-scaling branch July 4, 2024 06:45
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.

Cellpose task: Output ROI table creation performance scaling
2 participants