spacetx / starfish

starfish: unified pipelines for image-based transcriptomics
https://spacetx-starfish.readthedocs.io/en/latest/
MIT License
221 stars 67 forks source link

Add min_distance parameter to peak_local_max call #2008

Closed iimog closed 7 months ago

iimog commented 8 months ago

Before this change, multiple local maxima were returned even within the min_distance if their values were identical. This lead to over-segmentation in some cases.

See #2006

berl commented 8 months ago

@nickeener @ctcisar do you have any recommendations for the CI fails above?

nickeener commented 8 months ago

@berl the failure appears to be in the starfish/test/full_pipelines/api/test_iss_api.py::test_iss_pipeline_cropped_data test which is not something that we ever messed with, sorry.

iimog commented 8 months ago

I found the problem in the iss test. Indeed, the assumption on the number of detected cells had to be updated from 6 to 4. I think this makes sense. The previous pipeline output was this: iss_old

The new pipeline output is this: iss_new

So the number of detected cells did change from 6 to 4.

iimog commented 8 months ago

The docker build could not install all dependencies via pip because of this error:

ERROR: botocore 1.32.6 has requirement urllib3<1.27,>=1.25.4; python_version < "3.10", but you'll have urllib3 2.0.7 which is incompatible.

Pinning urllib3 to that range in REQUIREMENTS fixes the build.

berl commented 7 months ago

this is awesome! all that work put in by many people making end-to-end tests of these methods did exactly what it was supposed to. I'll review and merge