Skip to content

Remove Cellpose ROI overlap check #889

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

Merged
merged 8 commits into from
Jan 8, 2025

Conversation

jluethi
Copy link
Collaborator

@jluethi jluethi commented Jan 8, 2025

Closes #779

I decided against a detailed further review of this performance. Given that it is often a bottleneck in performance and its only value is a warning, I suggest we remove the check completely. We can reintroduce more relevant checks later if needed.

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

@jluethi jluethi marked this pull request as ready for review January 8, 2025 14:07
Copy link

github-actions bot commented Jan 8, 2025

Coverage report

This PR does not seem to contain any modification to coverable code.

@jluethi
Copy link
Collaborator Author

jluethi commented Jan 8, 2025

@tcompa 2 brief CI questions:

  1. It looks like we aren't testing the get_overlapping_pairs_3D anywhere else. I'm a bit hesitant to remove it, but it's not worth the trouble of adding specific tests for a function we aren't using. Opinions?
  2. Any idea why our CI now thinks we only have a 30% coverage rate? Is it looking for 7k lines of something we aren't supposed to be testing? Would be useful to fix

@tcompa
Copy link
Collaborator

tcompa commented Jan 8, 2025

Any idea why our CI now thinks we only have a 30% coverage rate? Is it looking for 7k lines of something we aren't supposed to be testing? Would be useful to fix

This is the issue which hit us during the ngio-task PR: coverage is counting a bunch of internal modules, see https://htmlpreview.github.io/?https://github.com/fractal-analytics-platform/fractal-tasks-core/blob/python-coverage-comment-action-data/htmlcov/index.html:
image

It's the same as #881. I'll have to debug it again, cause the first round was not successful.

@tcompa
Copy link
Collaborator

tcompa commented Jan 8, 2025

It looks like we aren't testing the get_overlapping_pairs_3D anywhere else. I'm a bit hesitant to remove it, but it's not worth the trouble of adding specific tests for a function we aren't using. Opinions?

It is used in https://github.com/fmi-basel/gliberal-scMultipleX/blob/1413c909588508d0a2d9bf3d423a5e4d3c3cb8f2/src/scmultiplex/fractal/surface_mesh_multiscale.py#L27, thus we cannot remove it.

I can quickly add a test, if it's useful.

@tcompa
Copy link
Collaborator

tcompa commented Jan 8, 2025

It is used in https://github.com/fmi-basel/gliberal-scMultipleX/blob/1413c909588508d0a2d9bf3d423a5e4d3c3cb8f2/src/scmultiplex/fractal/surface_mesh_multiscale.py#L27, thus we cannot remove it.

This obviously change if we plan to also propose a PR that removes that feature from scmultiplex (and if we think nobody else is using it).

@jluethi
Copy link
Collaborator Author

jluethi commented Jan 8, 2025

Hmm, multiple tasks use Cellpose as a template. Besides scmultiplex, it's also the private rdcnet task that is also using this function.

I think we should not remove the function before the fractal-tasks-core 2.0 update, as it would break multiple other packages. But we should stop using it ourselves in the cellpose task.

@jluethi
Copy link
Collaborator Author

jluethi commented Jan 8, 2025

I can quickly add a test, if it's useful.

If we have a good setup to add a basic test, nothing against that. But given that we may deprecate it anyway later, let's not spend too much time on that

@tcompa
Copy link
Collaborator

tcompa commented Jan 8, 2025

The test is now in-place.

The coverage message is obviously wrong, but this is due to my other activities (outside this PR).

In principle you can already merge this PR, since I only added a (passing) test to the previous PR state. I'm still working on fixing the coverage issue, but that's unrelated.

@jluethi
Copy link
Collaborator Author

jluethi commented Jan 8, 2025

Thanks a lot @tcompa !

@jluethi jluethi merged commit 6d77bb9 into main Jan 8, 2025
20 checks passed
@jluethi jluethi deleted the 779_deactivate_cellpose_overlap_check branch January 8, 2025 16:23
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.

Review/optimize use of get_overlapping_pairs_3D in Cellpose task
2 participants