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

Make map/reduce APIs more intuitive #1686

Merged
merged 1 commit into from
Dec 13, 2019
Merged

Conversation

ttung
Copy link
Collaborator

@ttung ttung commented Dec 5, 2019

Right now, specifying a FunctionSource along with function parameters makes for a confusing function call.

For instance, Map('divide', 2, module=FunctionSource.np) means the FunctionSource comes last, which is not intuitive.

This adds the ability for FunctionSources to be called and return a Bundle that includes both the package name and the function name. The call above would then become: Map(FunctionSource.np('divide'), 2).

Backwards compatibility with the prior API is maintained, but a warning is generated.

If the top-level package is provided twice, it is treated as an error.

Test plan: added test cases to cover the new approach, the old approach that should generate the warning, and the ugly combination that should fail.

@ttung ttung requested a review from shanaxel42 December 5, 2019 20:01
@ttung ttung force-pushed the tonytung-simplify-functionsource branch from 80ca1cf to 9c3fc18 Compare December 5, 2019 20:17
@codecov-io
Copy link

codecov-io commented Dec 5, 2019

Codecov Report

Merging #1686 into master will decrease coverage by 0.02%.
The diff coverage is 89.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1686      +/-   ##
==========================================
- Coverage   89.79%   89.76%   -0.03%     
==========================================
  Files         239      239              
  Lines        8975     9020      +45     
==========================================
+ Hits         8059     8097      +38     
- Misses        916      923       +7
Impacted Files Coverage Δ
starfish/types.py 100% <ø> (ø) ⬆️
starfish/core/imagestack/imagestack.py 92.04% <ø> (ø) ⬆️
...h/core/spots/FindSpots/test/test_spot_detection.py 100% <100%> (ø) ⬆️
...core/spots/DecodeSpots/test/test_trace_builders.py 100% <100%> (ø) ⬆️
...h/core/spots/FindSpots/test/test_synthetic_data.py 100% <100%> (ø) ⬆️
starfish/core/image/Filter/reduce.py 95% <100%> (+1.45%) ⬆️
starfish/core/types/__init__.py 100% <100%> (ø) ⬆️
starfish/core/types/_functionsource.py 90.62% <100%> (+2.16%) ⬆️
starfish/core/image/Filter/test/test_reduce.py 98.41% <100%> (+0.23%) ⬆️
starfish/core/image/Filter/test/test_map.py 100% <100%> (ø) ⬆️
... and 2 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 3584c57...4e9ccad. Read the comment docs.

@ttung ttung force-pushed the tonytung-simplify-functionsource branch 2 times, most recently from 47676a9 to 43adfd3 Compare December 6, 2019 18:58
Right now, specifying a FunctionSource along with function parameters makes for a confusing function call.

For instance, `Map('divide', 2, module=FunctionSource.np)` means the FunctionSource comes last, which is not intuitive.

This adds the ability for `FunctionSource`s to be called and return a Bundle that includes both the package name and the function name.  The call above would then become: `Map(FunctionSource.np('divide'), 2)`.

Backwards compatibility with the prior API is maintained, but a warning is generated.

If the top-level package is provided twice, it is treated as an error.

Test plan: added test cases to cover the new approach, the old approach that should generate the warning, and the ugly combination that should fail.
@ttung ttung force-pushed the tonytung-simplify-functionsource branch from 43adfd3 to 4e9ccad Compare December 11, 2019 18:38
@shanaxel42
Copy link
Collaborator

I actually didn't find the old method confusing. Was it breaking anything? I don't have a huge preference if you think this makes more sense but seems like a lot of changes

@ttung
Copy link
Collaborator Author

ttung commented Dec 11, 2019

It gets even more confusing when the function takes kwargs, because it becomes:

Reduce('function_name', function_positonal_arg0, function_positonal_arg1, module=FunctionSource.blahblah, function_kwarg_0=val, function_kwarg_1=val)

@shanaxel42
Copy link
Collaborator

It gets even more confusing when the function takes kwargs, because it becomes:

Reduce('function_name', function_positonal_arg0, function_positonal_arg1, module=FunctionSource.blahblah, function_kwarg_0=val, function_kwarg_1=val)

Ahhh ok then I support this.

@ttung ttung merged commit a15237c into master Dec 13, 2019
@ttung ttung deleted the tonytung-simplify-functionsource branch December 13, 2019 01:31
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