Unify FunctionSource in Map and Reduce #1540

ttung · 2019-09-06T22:52:23Z

This is needed to enable #1539, as that imports FunctionSource from ImageStack (and Map/Reduce import ImageStack).

Note that this is not entirely for free. The function aliases (example: where we alias 'max' to 'amax' for FunctionSource.np) now come from one source, which may or may not be desirable.

Test plan: travis yo

codecov-io · 2019-09-07T08:14:00Z

Codecov Report

Merging #1540 into master will increase coverage by <.01%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
+ Coverage   87.48%   87.49%   +<.01%     
==========================================
  Files         145      146       +1     
  Lines        5058     5036      -22     
==========================================
- Hits         4425     4406      -19     
+ Misses        633      630       -3

Impacted Files	Coverage Δ
starfish/types.py	`100% <ø> (ø)`	⬆️
starfish/core/image/Filter/reduce.py	`96.42% <100%> (+4.12%)`	⬆️
starfish/core/types/__init__.py	`100% <100%> (ø)`	⬆️
starfish/core/image/Filter/map.py	`100% <100%> (+7.31%)`	⬆️
starfish/core/types/_functionsource.py	`88% <88%> (ø)`

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 505f73c...bc1c1ec. Read the comment docs.

shanaxel42

general question, and sorry if this has already been discussed. But what's the advantage of having the function source logic over just have a function parameter passed into reduce? Like func=np.max

ttung · 2019-09-17T00:38:26Z

general question, and sorry if this has already been discussed. But what's the advantage of having the function source logic over just have a function parameter passed into reduce? Like func=np.max

as discussed offline, allowing arbitrary callables to be passed in makes provenance tracking extremely challenging. However, we could plausibly use code inspection to solve that problem.

ambrosejcarr · 2019-09-19T17:57:39Z

as discussed offline, allowing arbitrary callables to be passed in makes provenance tracking extremely challenging. However, we could plausibly use code inspection to solve that problem.

If you see a path where this is achievable, I believe usability benefits strongly from just passing in the function.

ttung · 2019-09-19T17:58:53Z

If you see a path where this is achievable, I believe usability benefits strongly from just passing in the function.

It's somewhat achievable. I'll look into it.

ambrosejcarr · 2019-09-19T18:05:47Z

It's somewhat achievable. I'll look into it.

After PR is merged is OK with me, if this blocks.

ttung · 2019-09-20T01:53:46Z

After PR is merged is OK with me, if this blocks.

#1568

Note that this is not entirely for free. The function aliases (example: where we alias 'max' to 'amax' for `FunctionSource.np`) now come from one source, which may or may not be desirable.

In #1540, I missed a conversion from Reduce.FunctionSource to types.FunctionSource.

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 requested review from ambrosejcarr and shanaxel42 September 6, 2019 22:52

ttung force-pushed the tonytung-map branch from 8666fe1 to c2bbb33 Compare September 6, 2019 23:11

ttung force-pushed the tonytung-unify-function-source branch from 36dbc07 to 5fbdb1d Compare September 6, 2019 23:11

ttung mentioned this pull request Sep 13, 2019

Add map/reduce methods to ImageStack #1539

Merged

ttung force-pushed the tonytung-unify-function-source branch from 5fbdb1d to a485f7a Compare September 13, 2019 17:22

shanaxel42 reviewed Sep 16, 2019

View reviewed changes

ttung force-pushed the tonytung-unify-function-source branch 2 times, most recently from beaad60 to 95bd2d1 Compare September 16, 2019 20:51

ttung force-pushed the tonytung-map branch from 5b81054 to 54c2a69 Compare September 16, 2019 20:52

ttung force-pushed the tonytung-unify-function-source branch from 95bd2d1 to ab3545d Compare September 16, 2019 20:53

ttung changed the base branch from tonytung-map to master September 16, 2019 20:55

ttung force-pushed the tonytung-unify-function-source branch from ab3545d to 079358f Compare September 16, 2019 20:57

ttung force-pushed the tonytung-unify-function-source branch 7 times, most recently from e1857ac to 947124f Compare September 18, 2019 22:53

ttung force-pushed the master branch from 6f8abb9 to c16c8de Compare September 18, 2019 23:39

ttung force-pushed the tonytung-unify-function-source branch 4 times, most recently from ed418fe to eb15634 Compare September 20, 2019 01:47

ttung force-pushed the tonytung-unify-function-source branch 4 times, most recently from 14d935b to 1b51899 Compare September 24, 2019 18:04

ttung force-pushed the master branch 2 times, most recently from 67a6870 to 849058e Compare September 24, 2019 18:10

ttung force-pushed the tonytung-unify-function-source branch 4 times, most recently from bdb0129 to c647b6d Compare October 1, 2019 05:06

ttung force-pushed the tonytung-unify-function-source branch 2 times, most recently from 93cbd18 to 1cd0b80 Compare October 4, 2019 17:53

ttung requested a review from shanaxel42 October 4, 2019 22:09

shanaxel42 approved these changes Oct 4, 2019

View reviewed changes

ttung force-pushed the tonytung-unify-function-source branch 2 times, most recently from bc1c1ec to c193ef5 Compare October 9, 2019 21:01

Unify FunctionSource in Map and Reduce

b042e2d

Note that this is not entirely for free. The function aliases (example: where we alias 'max' to 'amax' for `FunctionSource.np`) now come from one source, which may or may not be desirable.

ttung force-pushed the tonytung-unify-function-source branch from c193ef5 to b042e2d Compare October 9, 2019 21:14

ttung merged commit 9592539 into master Oct 9, 2019

ttung mentioned this pull request Oct 9, 2019

Fix broken build #1608

Merged

ttung pushed a commit that referenced this pull request Oct 9, 2019

Fix broken build

757bc08

In #1540, I missed a conversion from Reduce.FunctionSource to types.FunctionSource.

ttung pushed a commit that referenced this pull request Oct 9, 2019

Fix broken build (#1608)

c9957ea

In #1540, I missed a conversion from Reduce.FunctionSource to types.FunctionSource.

ttung deleted the tonytung-unify-function-source branch October 10, 2019 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify FunctionSource in Map and Reduce #1540

Unify FunctionSource in Map and Reduce #1540

ttung commented Sep 6, 2019

codecov-io commented Sep 7, 2019 •

edited

Loading

shanaxel42 left a comment

ttung commented Sep 17, 2019

ambrosejcarr commented Sep 19, 2019

ttung commented Sep 19, 2019

ambrosejcarr commented Sep 19, 2019

ttung commented Sep 20, 2019

Unify FunctionSource in Map and Reduce #1540

Unify FunctionSource in Map and Reduce #1540

Conversation

ttung commented Sep 6, 2019

codecov-io commented Sep 7, 2019 • edited Loading

Codecov Report

shanaxel42 left a comment

Choose a reason for hiding this comment

ttung commented Sep 17, 2019

ambrosejcarr commented Sep 19, 2019

ttung commented Sep 19, 2019

ambrosejcarr commented Sep 19, 2019

ttung commented Sep 20, 2019

codecov-io commented Sep 7, 2019 •

edited

Loading