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

Maintenance to clip and threshold application arguments #44

Merged
merged 5 commits into from Feb 17, 2023

Conversation

elaubsch
Copy link
Member

@elaubsch elaubsch commented Feb 8, 2023

This PR addresses a naming discrepancy between the SpotDetection and Polaris applications for the clip and threshold parameters. The default value for clip in SpotDetection and Polaris have now been set to True, because this setting gives better spot detection results on a wider range of images.

It also removes a line passing a threshold argument into the SpotDetection application inside the Polaris application. There are two reasons for this: (1) the SpotDetection application in Polaris is instantiated with postprocessing_fn=None, so the threshold argument would not be used, and (2) the default value for threshold in the SpotDetection application prevents an error from being raised about its value.

@elaubsch elaubsch added the chore Maintenance label Feb 8, 2023
@elaubsch elaubsch requested a review from rossbar February 8, 2023 00:15
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, just one comment about updating the docstring parameter descriptions to match the change in the default.

considered as a spot.
spots_clip (bool): Determines if pixel values will be clipped by percentile.
clip (bool): Determines if pixel values will be clipped by percentile.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC the line below should be updated to Defaults to true. Same with the instance above in the _predict_spots_image docstring (I think)

@elaubsch elaubsch merged commit 6eef203 into master Feb 17, 2023
@elaubsch elaubsch deleted the mly/app-args branch February 17, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants