8000 Add map/reduce methods to ImageStack by ttung · Pull Request #1539 · spacetx/starfish · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
/ starfish Public

Add map/reduce methods to ImageStack #1539

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2019
Merged

Add map/reduce methods to ImageStack #1539

merged 1 commit into from
Oct 10, 2019

Conversation

ttung
Copy link
Collaborator
@ttung ttung commented Sep 5, 2019 •
edited
Loading

This is syntactic sugar to simplify single map / reduce operations. Right now, we would need to instantiate the filter and then run it against the stack, e.g.,

max_projector = Filter.Reduce((Axes.CH, Axes.ROUND, Axes.ZPLANE))
max_projected = max_projector.run(stack)

With this, it simplifies to:

max_projected = stack.reduce((Axes.CH, Axes.ROUND, Axes.ZPLANE), "max")

Test plan: Converted imagestack/test/test_max_proj.py to use this idiom.
Depends on #1540

@codecov-io
Copy link
codecov-io commented Sep 5, 2019 •
edited
Loading

Codecov Report

Merging #1539 into master will decrease coverage by 0.03%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1539      +/-   ##
=========================================
- Coverage   88.04%     88%   -0.04%     
=========================================
  Files         150     150              
  Lines        5143    5151       +8     
=========================================
+ Hits         4528    4533       +5     
- Misses        615     618       +3
Impacted Files Coverage Δ
starfish/core/image/Filter/reduce.py 96.42% <ø> (ø) ⬆️
starfish/core/imagestack/imagestack.py 93.69% <62.5%> (-0.7%) ⬇️

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 c9957ea...5be3a3e. Read the comment docs.

@ttung ttung force-pushed the tonytung-unify-function-source branch from 58ea189 to 43938cd Compare September 5, 2019 19:46
@ttung ttung force-pushed the tonytung-map-reduce branch 3 times, most recently from 7220df8 to 6d54cfe Compare September 5, 2019 21:46
ambrosejcarr
ambrosejcarr reviewed Sep 5, 2019
View reviewed changes
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.

Generally I want to follow the principles that starfish should be "simple and intuitive" I think this improves on the package, and follows patterns our users would expect to see from numpy, xarray, and pandas, where common methods are accessible from the main objects.

starfish/core/imagestack/imagestack.py
@@ -50,6 +51,7 @@
Clip,
Coordinates,
CoordinateValue,
FunctionSource,
Copy link
Member

Choose a reason for hiding this comment

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

Meta-comment: I think logging and reproducibility is very important, but wonder if we're shooting ourselves in the foot re: usability by not accepting functions directly. I know last time we discussed we waffled a bit and then decided on the side of logging.

How would you feel if @neuromusic or I did some research on this question to get some signal on user expectations and whether this approach is intuitive enough for our users to understand?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Meta-comment: I think logging and reproducibility is very important, but wonder if we're shooting ourselves in the foot re: usability by not accepting functions directly. I know last time we discussed we waffled a bit and then decided on the side of logging.

I still think that is the right choice. Otherwise, starfish doesn't have much of value-add.

starfish/core/imagestack/imagestack.py
@@ -1215,3 +1217,33 @@ def max_proj(self, *dims: Axes) -> "ImageStack":
def _squeezed_numpy(self, *dims: Axes):
"""return this ImageStack's data as a squeezed numpy array"""
return self.xarray.squeeze(tuple(dim.value for dim in dims)).values

def reduce(
Copy link
Member

Choose a reason for hiding this comment

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

definitely will need docs. :-)

starfish/core/imagestack/imagestack.py
reducer = Filter.Reduce(dims, func, module, clip_method, **kwargs)
return reducer.run(self, *args)

def map(
Copy link
Member

Choose a reason for hiding this comment

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

definitely will need docs. :-)

@ttung ttung mentioned this pull request Sep 6, 2019
Merged
@ttung ttung force-pushed the tonytung-map-reduce branch 2 times, most recently from 7ef87a6 to 47d990f Compare September 11, 2019 21:48
@ttung ttung requested a review from ambrosejcarr September 11, 2019 21:49
@ttung
Copy link
Collaborator Author
ttung commented Sep 11, 2019

This is ready for review again.

@ttung ttung force-pushed the tonytung-map-reduce branch from 47d990f to d201816 Compare September 11, 2019 22:41
@ttung ttung mentioned this pull request Sep 13, 2019
Merged
@ttung ttung changed the title RFC: Add map/reduce methods to ImageStack Add map/reduce methods to ImageStack Sep 13, 2019
@ttung ttung force-pushed the tonytung-map-reduce branch from d201816 to 181fdd3 Compare September 13, 2019 17:09
@ttung ttung force-pushed the tonytung-unify-function-source branch from 5fbdb1d to a485f7a Compare September 13, 2019 17:22
@ttung ttung force-pushed the tonytung-map-reduce branch from 181fdd3 to 18ebbc3 Compare September 13, 2019 17:22
@ttung ttung force-pushed the tonytung-map-reduce branch from 18ebbc3 to cf4f2ec Compare September 16, 2019 19:16
@ttung ttung force-pushed the tonytung-unify-function-source branch from beaad60 to 95bd2d1 Compare September 16, 2019 20:51
@ttung ttung force-pushed the tonytung-map-reduce branch 3 times, most recently from df5f44a to 5534128 Compare September 16, 2019 20:57
@ttung ttung force-pushed the tonytung-unify-function-source branch from 079358f to f50b31c Compare September 17, 2019 00:40
@ttung ttung force-pushed the tonytung-map-reduce branch from 5534128 to e5ba35e Compare September 17, 2019 00:40
@ttung ttung force-pushed the tonytung-unify-function-source branch from f50b31c to 9e280f3 Compare September 18, 2019 17:56
@ttung ttung force-pushed the tonytung-map-reduce branch from e5ba35e to 1436e1e Compare September 18, 2019 17:56
@ttung ttung force-pushed the tonytung-unify-function-source branch from 9e280f3 to f79d217 Compare September 18, 2019 18:57
@ttung ttung force-pushed the tonytung-map-reduce branch from 0bfbd65 to 64d5322 Compare September 20, 2019 01:09
@ttung ttung force-pushed the tonytung-unify-function-source branch from ed418fe to eb15634 Compare September 20, 2019 01:47
@ttung ttung force-pushed the tonytung-map-reduce branch 2 times, most recently from 4bd8dfe to 511867e Compare September 23, 2019 18:32
@ttung ttung force-pushed the tonytung-unify-function-source branch from 1a919c9 to ffce74c Compare September 23, 2019 23:24
@ttung ttung force-pushed the tonytung-map-reduce branch 4 times, most recently from df2ba19 to de1c6fe Compare September 24, 2019 18:12
@ttung ttung force-pushed the tonytung-unify-function-source branch from 17c1650 to a6179ea Compare September 27, 2019 17:53
@ttung ttung force-pushed the tonytung-map-reduce branch 2 times, most recently from 9e47685 to e82ef53 Compare October 1, 2019 03:18
@ttung ttung force-pushed the tonytung-unify-function-source branch from bdb0129 to c647b6d Compare October 1, 2019 05:06
@ttung ttung force-pushed the tonytung-map-reduce branch 3 times, most recently from f08a569 to 2210ee2 Compare October 4, 2019 17:53
@ttung ttung force-pushed the tonytung-unify-function-source branch from 1cd0b80 to bc1c1ec Compare October 8, 2019 22:31
@ttung ttung force-pushed the tonytung-map-reduce branch 2 times, most recently from b7d9351 to da9a140 Compare October 9, 2019 20:22
@ttung ttung force-pushed the tonytung-unify-function-source branch from bc1c1ec to c193ef5 Compare October 9, 2019 21:01
@ttung ttung force-pushed the tonytung-map-reduce branch from da9a140 to 5f3aa70 Compare October 9, 2019 21:01
@ttung ttung force-pushed the tonytung-unify-function-source branch from c193ef5 to b042e2d Compare October 9, 2019 21:14
@ttung ttung force-pushed the tonytung-map-reduce branch from 5f3aa70 to 3c39f9f Compare October 9, 2019 21:14
@ttung ttung changed the base branch from tonytung-unify-function-source to master October 9, 2019 21:21
@ttung ttung force-pushed the tonytung-map-reduce branch 2 times, most recently from 5862e96 to 637236b Compare October 9, 2019 23:56
Add map/reduce methods to ImageStack
5be3a3e 
This is syntactic sugar to simplify single map / reduce operations.  Right now, we would need to instantiate the filter and then run it against the stack, e.g.,

```
max_projector = Filter.Reduce((Axes.CH, Axes.ROUND, Axes.ZPLANE))
max_projected = max_projector.run(stack)
```

With this, it simplifies to:

```
max_projected = stack.reduce((Axes.CH, Axes.ROUND, Axes.ZPLANE), "max")
```

Test plan: Converted imagestack/test/test_max_proj.py to use this idiom.
Depends on #1540
@ttung ttung force-pushed the tonytung-map-reduce branch from 637236b to 5be3a3e Compare October 10, 2019 00:03
@ttung ttung merged commit a958db6 into master Oct 10, 2019
@ttung ttung deleted the tonytung-map-reduce branch October 10, 2019 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shanaxel42 shanaxel42 shanaxel42 approved these changes

@ambrosejcarr ambrosejcarr Awaiting requested review from ambrosejcarr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ttung @codecov-io @ambrosejcarr @shanaxel42
0