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

Add Filter.Reduce (general dimension reduction for ImageStack) #1342

Merged
merged 14 commits into from
May 30, 2019

Conversation

kevinyamauchi
Copy link
Collaborator

@kevinyamauchi kevinyamauchi commented May 13, 2019

Summary

As discussed in #1302, this component provides the ability to perform dimension reduction operations to ImageStack using DataArray.reduce(). Currently, max, mean, sum, and a user-provided can be applied across specified dimensions. Closes #1302.

Testing

Testing of sum, mean, and max are performed in starfish/core/image/_filter/test/test_reduce.py

To Do

  • Add tests
  • Update physical coordinates

@codecov-io
Copy link

codecov-io commented May 13, 2019

Codecov Report

Merging #1342 into master will increase coverage by 0.04%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1342      +/-   ##
==========================================
+ Coverage   89.21%   89.25%   +0.04%     
==========================================
  Files         148      149       +1     
  Lines        5377     5416      +39     
==========================================
+ Hits         4797     4834      +37     
- Misses        580      582       +2
Impacted Files Coverage Δ
starfish/core/image/_filter/reduce.py 94.87% <94.87%> (ø)

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 618e891...161e5a3. Read the comment docs.

starfish/core/image/_filter/reduce.py Show resolved Hide resolved
starfish/core/image/_filter/reduce.py Outdated Show resolved Hide resolved
----------
dims : Axes
one or more Axes to project over
func : Union[str, Callable]
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not accept just Callable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i like it. this is the more 'pythonic' way in scientific computing -- will be easier for numpy/pandas users to reason about i think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dganguli You like what? The original or my suggestion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the absence of an opposition, I think func should be a Callable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's nice to have the string option. I think a lot of users will find it annoying to need to import numpy.amax() and this component to perform a maximum intensity projection.

Copy link
Member

Choose a reason for hiding this comment

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

I support the string option, just for ecosystem continuity -- users are used to passing function names that are resolved by getattr, as this formalism is used a lot in scipy.spatial.distance among other packages.

starfish/core/image/_filter/reduce.py Outdated Show resolved Hide resolved
starfish/core/image/_filter/reduce.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dganguli dganguli left a comment

Choose a reason for hiding this comment

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

this is awesome. is the plan to re-write max_proj to call reduce with 'max' or 'np.max'? or are we ditching max_proj?

@kevinyamauchi
Copy link
Collaborator Author

@dganguli , I think @ttung suggested that we keep Filter.max_proj() and ImageStack.max_proj() for a bit for API stability. I would be in favor of removing ImageStack.max_proj() and refactoring Filter.max_proj() to use Filter.reduce(). I think it's worth keeping Filter.max_proj() around because maximum intensity projection is such a common operation. Thoughts?

@kevinyamauchi
Copy link
Collaborator Author

@ttung , I think I've addressed your comments. However, the test is failing during doc generation (see error below) and I'm not sure what it is expecting because it passes flake8

Warning, treated as error:
/home/travis/build/spacetx/starfish/starfish/core/image/_filter/reduce.py:docstring of starfish.image.Filter.Reduce:21:Unexpected indentation.
make[1]: *** [html] Error 2
make[1]: Leaving directory `/home/travis/build/spacetx/starfish/docs'
make: *** [docs-html] Error 2
The command "make install-dev fast" exited with 2.

@kevinyamauchi kevinyamauchi requested review from ttung and dganguli May 29, 2019 19:30
@kevinyamauchi
Copy link
Collaborator Author

@ttung, I think I have addressed your comments. Would you mind taking another look?

@kevinyamauchi kevinyamauchi changed the title [WIP] Add Filter.Reduce (general dimension reduction for ImageStack) Add Filter.Reduce (general dimension reduction for ImageStack) May 30, 2019
----------
dims : Axes
one or more Axes to project over
func : Union[str, Callable]
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the absence of an opposition, I think func should be a Callable.

starfish/core/image/_filter/reduce.py Outdated Show resolved Hide resolved
starfish/core/image/_filter/reduce.py Outdated Show resolved Hide resolved
@kevinyamauchi kevinyamauchi merged commit d3f3a84 into master May 30, 2019
@kevinyamauchi kevinyamauchi deleted the ky-add-filter-reduce branch May 30, 2019 22:18
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.

Add support for other types of projections on ImageStack
5 participants