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

Move preserve_float_range #1041

Merged
merged 10 commits into from
Mar 2, 2019
Merged

Conversation

ambrosejcarr
Copy link
Member

@ambrosejcarr ambrosejcarr commented Feb 22, 2019

Purpose

Clipping and scaling was not happening properly in our filters, leading to some bugs. Specifically, preserve_float_range with rescale=True scales by the max value of the provided image. This was being called on each chunk independently, when some filters need to call it on the whole imagestack. This PR adds that flexibility.

Changes

  • Move preserve_float_range calls into ImageStack.apply and enable 3 modes of clipping (see below).
  • Move preserve_float_range from filter to starfish.util.dtype.
  • Each filter that can create values outside [0, 1] now receives a clip_method parameter.
  • Add comments to apply, transform and related methods to clarify process flow.
  • Add tests for clipping types
  • Improve correlation for MERFISH notebook by adjusting deconvolution clipping
  • Fix miscellaneous documentation errors in the Filters

New clip_method parameter:

        clip_method : int
            (Default 0) Controls the way that data are scaled to retain skimage dtype
            requirements that float data fall in [0, 1].
            0: data above 1 are set to 1, and below 0 are set to 0
            1: data above 1 are scaled by the maximum value, with the maximum value calculated
               over the entire ImageStack
            2: data above 1 are scaled by the maximum value, with the maximum value calculated
               over each slice, where slice shapes are determined by the group_by parameters

Review strategy

  • review starfish/imagestack/imagestack.py
  • review starfish/test/image/test_apply.py
  • review starfish/test/full_pipelines/api/test_merfish.py
  • glance at all the filters and the merfish notebook -- changes there are minimal.

@codecov-io
Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #1041 into master will increase coverage by 0.09%.
The diff coverage is 92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1041      +/-   ##
==========================================
+ Coverage   88.39%   88.48%   +0.09%     
==========================================
  Files         163      164       +1     
  Lines        6090     6139      +49     
==========================================
+ Hits         5383     5432      +49     
  Misses        707      707
Impacted Files Coverage Δ
...tarfish/image/_filter/zero_by_channel_magnitude.py 100% <ø> (ø) ⬆️
starfish/spots/_detector/detect.py 98.36% <ø> (ø) ⬆️
starfish/image/_filter/util.py 83.33% <ø> (-1.78%) ⬇️
starfish/types/__init__.py 100% <ø> (ø) ⬆️
starfish/types/_constants.py 97.82% <100%> (+0.2%) ⬆️
starfish/image/_filter/white_tophat.py 100% <100%> (ø) ⬆️
starfish/test/image/test_apply.py 100% <100%> (ø) ⬆️
starfish/test/pipeline/test_filter.py 100% <100%> (ø) ⬆️
starfish/intensity_table/intensity_table.py 82.2% <100%> (ø) ⬆️
starfish/image/_filter/scale_by_percentile.py 100% <100%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35021df...5d6f2fb. Read the comment docs.

@ambrosejcarr ambrosejcarr mentioned this pull request Feb 23, 2019
@ambrosejcarr ambrosejcarr force-pushed the ajc-move-preserve-float-range branch from 12adec9 to bb3634b Compare February 23, 2019 20:23
starfish/image/_filter/bandpass.py Outdated Show resolved Hide resolved
starfish/spots/_detector/detect.py Outdated Show resolved Hide resolved
sorted(AXES_DATA.items(), key=lambda kv: kv[1].order)
] + [Axes.Y.value, Axes.X.value]

# build and then slice the xarray to get the piece needed for this worker
Copy link
Collaborator

Choose a reason for hiding this comment

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

This drops the coords, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the filtered function, yes. From the resulting object, no.

Copy link
Collaborator

@shanaxel42 shanaxel42 left a comment

Choose a reason for hiding this comment

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

Make the enum change but otherwise looks good!

@ambrosejcarr ambrosejcarr force-pushed the ajc-move-preserve-float-range branch from bb3634b to 07cb43d Compare March 2, 2019 03:05
@ambrosejcarr ambrosejcarr force-pushed the ajc-move-preserve-float-range branch from 07cb43d to 4b8c85a Compare March 2, 2019 03:09
@ambrosejcarr ambrosejcarr requested a review from ttung March 2, 2019 03:11
@ambrosejcarr ambrosejcarr merged commit ae5311a into master Mar 2, 2019
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.

4 participants