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

Refactor blobs_image api. #1143

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Refactor blobs_image api. #1143

merged 1 commit into from
Apr 8, 2019

Conversation

ttung
Copy link
Collaborator

@ttung ttung commented Apr 7, 2019

High level goal: detect spots should accept imagestacks and not numpy arrays.

  1. blobs_image is, at the user's interaction level, an ImageStack. Internally, starfish.spots._detector.detect.detect_spots will convert it to a numpy array. This is to allow the code path where we don't have a reference image to continue taking advantage of ImageStack's apply/transform logic.
  2. Because the way we generate the blobs_image (max projection) happens on different axes, it must be explicitly provided to detect_spots.
  3. Other small and insignificant changes.

Test plan: make -j test run_notebooks

@ttung ttung requested review from ambrosejcarr and shanaxel42 April 7, 2019 16:54
@ttung ttung force-pushed the tonytung-blobs-image branch from bafdf4c to 3b1cc1e Compare April 7, 2019 16:54
@codecov-io
Copy link

codecov-io commented Apr 7, 2019

Codecov Report

Merging #1143 into master will increase coverage by <.01%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1143      +/-   ##
==========================================
+ Coverage   88.62%   88.63%   +<.01%     
==========================================
  Files         130      130              
  Lines        4642     4637       -5     
==========================================
- Hits         4114     4110       -4     
+ Misses        528      527       -1
Impacted Files Coverage Δ
starfish/util/click/__init__.py 96.29% <ø> (ø) ⬆️
starfish/test/full_pipelines/cli/test_iss.py 100% <ø> (ø) ⬆️
starfish/spots/_detector/local_max_peak_finder.py 52.94% <ø> (ø) ⬆️
starfish/spots/_detector/blob.py 96.15% <100%> (ø) ⬆️
...h/spots/_detector/trackpy_local_max_peak_finder.py 91.66% <100%> (ø) ⬆️
starfish/spots/_detector/_base.py 90.38% <100%> (+1.29%) ⬆️
starfish/spots/_detector/detect.py 98.3% <75%> (-0.06%) ⬇️

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 911f024...3b1cc1e. Read the comment docs.

Copy link
Member

@ambrosejcarr ambrosejcarr left a comment

Choose a reason for hiding this comment

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

Great change, nice to eliminate the extra work currently required of users. Thanks.

… arrays.

1. blobs_image is, at the user's interaction level, an ImageStack.  Internally, `starfish.spots._detector.detect.detect_spots` will convert it to a numpy array.  This is to allow the code path where we don't have a reference image to continue taking advantage of ImageStack's apply/transform logic.
2. Because the way we generate the blobs_image (max projection) happens on different axes, it must be explicitly provided to `detect_spots`.
3. Other small and insignificant changes.

Test plan: `make -j test run_notebooks`
@ttung ttung force-pushed the tonytung-blobs-image branch from 3b1cc1e to 19c5d18 Compare April 8, 2019 07:35
@ttung ttung merged commit 04c78ba into master Apr 8, 2019
@ttung ttung deleted the tonytung-blobs-image branch April 8, 2019 07:36
ttung pushed a commit that referenced this pull request Apr 8, 2019
#1124 ensures that blobs_stack is already an ImageStack, but #1143 still does the conversion.  Merging both created this conflict.

Test plan: `pytest starfish/test/full_pipelines/cli/test_iss.py`
ttung added a commit that referenced this pull request Apr 8, 2019
#1124 ensures that blobs_stack is already an ImageStack, but #1143 still does the conversion.  Merging both created this conflict.

Test plan: `pytest starfish/test/full_pipelines/cli/test_iss.py`
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.

3 participants