Imagestack.transform executes applied function on xarray objects instead of numpy arrays #1035

ambrosejcarr · 2019-02-21T01:00:31Z

@kevinyamauchi pointed out in #987 and #983 that our apply method hampers our ability to write expressive PipelineComponents because it returns numpy arrays instead of xarray DataArray objects.

This PR:

Modifies ImageStack.transform so that methods used by apply can take advantage of xarray named axes. Because we are rebuilding an array from the sharedmemory object, this does not give us access to data stored in the coordinates. Thus, the returned arrays only guarantee that the Axes are in the correct order. If is_volume is True, xarrays generated by ImageStack.transform have dims = (z, y, x) and no coordinates. If is_volume is False, xarrays generated by ImageStack.transform have dims = (y, x) and no coordinates. These are set based on the information in starfish.imagestack.dataorder.AXES_DATA
Fixes a bug in preserve_float_range. That function has obviously never seen an xarray before. 😆
Minor updates to SpotFinder methods to support receiving xr.DataArray objects
Updates to docstrings to reflect flexibility to receive Union[xr.DataArray, np.ndarray] images.

codecov-io · 2019-02-21T01:03:26Z

Codecov Report

Merging #1035 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1035      +/-   ##
==========================================
+ Coverage    88.7%   88.74%   +0.03%     
==========================================
  Files         164      164              
  Lines        6036     6048      +12     
==========================================
+ Hits         5354     5367      +13     
+ Misses        682      681       -1

Impacted Files	Coverage Δ
starfish/image/_filter/gaussian_high_pass.py	`96.87% <ø> (ø)`	⬆️
starfish/image/_filter/scale_by_percentile.py	`100% <100%> (ø)`	⬆️
starfish/spots/_detector/detect.py	`96.82% <100%> (-0.05%)`	⬇️
...ish/image/_filter/richardson_lucy_deconvolution.py	`94.44% <100%> (+0.1%)`	⬆️
starfish/image/_filter/white_tophat.py	`100% <100%> (ø)`	⬆️
...h/spots/_detector/trackpy_local_max_peak_finder.py	`91.8% <100%> (+0.27%)`	⬆️
starfish/imagestack/imagestack.py	`84% <100%> (+0.03%)`	⬆️
starfish/image/_filter/util.py	`85.71% <100%> (+2.04%)`	⬆️
starfish/image/_filter/bandpass.py	`94.59% <100%> (+0.15%)`	⬆️
starfish/image/_filter/mean_high_pass.py	`96.87% <100%> (+0.1%)`	⬆️
... and 4 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 7998996...5119aa4. Read the comment docs.

kevinyamauchi · 2019-02-21T07:05:13Z

Thanks, @ambrosejcarr! I am not very experienced with multiprocessing, so I think I should leave the approval to @shanaxel42.

I did have a question though: in general, why do the filtering functions in all of the filters retain compatibility for image being passed in as an ndarray?

ambrosejcarr · 2019-02-21T13:30:11Z

I did have a question though: in general, why do the filtering functions in all of the filters retain compatibility for image being passed in as an ndarray?

Not all of them did, but this is by design: xarray wraps a numpy array and implements or delegates most of the numpy array functionality. Pretty cool right? :-)

kevinyamauchi · 2019-02-21T17:54:15Z

Sorry for being unclear. I was more specifically wondering if the fact that the type hint for image in many of the filtering methods (e.g., Clip._clip()) include ndarray implies that there are conditions in which ImageStack._processing_workflow() would pass an ndarray instead of an xarray. Or is it just to indicate the compatibility as you described above?

ambrosejcarr · 2019-02-21T17:55:08Z

is it just to indicate the compatibility as you described above?

Just to indicate compatibility. 👍

There is also the case that someone calls the private method with a numpy array. The only execution flow that I'm aware of in the codebase now involves xarray objects.

kevinyamauchi · 2019-02-21T17:57:27Z

Got it. Thanks!

shanaxel42

looks good, just a few comments

shanaxel42 · 2019-02-21T19:57:56Z

starfish/image/_filter/util.py

    if np.any(data < 0):
-        data[array < 0] = 0
-    if np.any(array > 1):
+        data[data < 0] = 0


why did this have to change?

🐛 !

If data is an xarray, we need to convert it to a numpy array to run these comparisons because indexing works differently between the two objects: http://xarray.pydata.org/en/stable/indexing.html#indexing-rules

This function was just never properly tested with an xarray input. np.ndarray[xr.DataArray] does not work.

shanaxel42 · 2019-02-21T20:06:21Z

starfish/imagestack/imagestack.py

+        dims = [
+            name.value for (name, data) in
+            sorted(AXES_DATA.items(), key=lambda kv: kv[1].order)
+        ] + [Axes.Y.value, Axes.X.value]


we're just trying to maintain the current axes order here right? why not just dims = [dim for dim in self.xarray.dims]

Yes, but this is a static method so does not have access to self. Happy to simplify this if you can find an easier way!

ohhhh I missed that. hmm well you could make dims_order a param you pass into _processing_workflow, then do then above in transform()... might be a little more clear

ttung · 2019-02-26T07:08:25Z

starfish/imagestack/imagestack.py


+        # build and then slice the xarray to get the piece needed for this worker


This does not set the xarray coordinates correctly. I imagine that if you use crop-on-load, your labels will all be incorrect.

There are some places in the codebase where we need a numpy array instead of an xarray. Switch all instances where we do this to `np.asarray(...)`. Also, fixed type hints where appropriate. This should fix the master-breaking bug in #1035, but does not resolve the issue of (xarray) coordinates not being transferred to the constituent xarrays.

ImageStack.apply uses slice indices based on the labels, but the xarray passed to the worker process does not have the labels. This PR passes the dims and the coordinates of the original xarray to the worker process so it can reconstitute an identical xarray. Test plan: Added a test that creates an ImageStack with labeled indices, and runs apply on it. Without the code change, it fails. With the code change, it works. Fixes #1108, and the comment raised in #1035 (comment) :)

ambrosejcarr added 2 commits February 20, 2019 19:46

modified transform to return an xarray, fixed any failing tests

f294454

fix type specifications

5119aa4

ambrosejcarr requested review from kevinyamauchi and shanaxel42 February 21, 2019 01:00

This was referenced Feb 21, 2019

[RFC] Add ElementWiseMult #983

Merged

[RFC] Add linear unmixing #987

Closed

shanaxel42 approved these changes Feb 21, 2019

View reviewed changes

ambrosejcarr merged commit ade276c into master Feb 23, 2019

ambrosejcarr deleted the ajc-transform-xarray branch February 23, 2019 19:18

ttung reviewed Feb 26, 2019

View reviewed changes

ttung mentioned this pull request Feb 26, 2019

Fix how we cast from xarray to numpy #1046

Merged

ambrosejcarr mentioned this pull request Mar 4, 2019

Pass image tiles as xarrays to function in ImageStack.apply() #1034

Closed

ttung mentioned this pull request Apr 11, 2019

Fix apply for ImageStacks with labeled indices. #1167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imagestack.transform executes applied function on xarray objects instead of numpy arrays #1035

Imagestack.transform executes applied function on xarray objects instead of numpy arrays #1035

ambrosejcarr commented Feb 21, 2019 •

edited

Loading

codecov-io commented Feb 21, 2019 •

edited

Loading

kevinyamauchi commented Feb 21, 2019

ambrosejcarr commented Feb 21, 2019 •

edited

Loading

kevinyamauchi commented Feb 21, 2019

ambrosejcarr commented Feb 21, 2019 •

edited

Loading

kevinyamauchi commented Feb 21, 2019

shanaxel42 left a comment

shanaxel42 Feb 21, 2019

ambrosejcarr Feb 21, 2019

shanaxel42 Feb 21, 2019

ambrosejcarr Feb 21, 2019 •

edited

Loading

shanaxel42 Feb 21, 2019

ttung Feb 26, 2019


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

Imagestack.transform executes applied function on xarray objects instead of numpy arrays #1035

Imagestack.transform executes applied function on xarray objects instead of numpy arrays #1035

Conversation

ambrosejcarr commented Feb 21, 2019 • edited Loading

codecov-io commented Feb 21, 2019 • edited Loading

Codecov Report

kevinyamauchi commented Feb 21, 2019

ambrosejcarr commented Feb 21, 2019 • edited Loading

kevinyamauchi commented Feb 21, 2019

ambrosejcarr commented Feb 21, 2019 • edited Loading

kevinyamauchi commented Feb 21, 2019

shanaxel42 left a comment

Choose a reason for hiding this comment

shanaxel42 Feb 21, 2019

Choose a reason for hiding this comment

ambrosejcarr Feb 21, 2019

Choose a reason for hiding this comment

shanaxel42 Feb 21, 2019

Choose a reason for hiding this comment

ambrosejcarr Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

shanaxel42 Feb 21, 2019

Choose a reason for hiding this comment

ttung Feb 26, 2019

Choose a reason for hiding this comment

ambrosejcarr commented Feb 21, 2019 •

edited

Loading

codecov-io commented Feb 21, 2019 •

edited

Loading

ambrosejcarr commented Feb 21, 2019 •

edited

Loading

ambrosejcarr commented Feb 21, 2019 •

edited

Loading

ambrosejcarr Feb 21, 2019 •

edited

Loading