Refactor reduce to take an optional module and only a function name. #1386
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1386 +/- ##
==========================================
- Coverage 89.42% 89.41% -0.02%
==========================================
Files 152 152
Lines 5531 5553 +22
==========================================
+ Hits 4946 4965 +19
- Misses 585 588 +3
Continue to review full report at Codecov.
|
|
A couple high-level comments:
module=scipy.linalg
function="norm"
kwargs={"ord": 2} |
|
This looks good to be. I agree with @ambrosejcarr that some examples in the documentation would be really helpful. |
ttung
force-pushed
the
tonytung-reduce
branch
2 times, most recently
from
162b3a5 to
714a795
Compare
|
I added a couple of tests, a couple of examples, and beefed up the docs. |
ttung
force-pushed
the
tonytung-reduce
branch
2 times, most recently
from
023cb4d to
fd66303
Compare
| @@ -17,53 +22,141 @@ | |||
|
|
|||
| class Reduce(FilterAlgorithmBase): | |||
| """ | |||
| Reduces the dimensions of the ImageStack by applying a function | |||
| along one or more axes. | |||
| Reduces the dimensions of the ImageStack by applying a function along one or more axes. | |||
There was a problem hiding this comment.
does 'Reduces the size of the dimensionsmake more sense? Reduces the dimensions made me think we were reducing the number of dimensions
There was a problem hiding this comment.
I reworded this. It now says, "Reduces the cardinality of one or more axes to 1 by applying a function across those axes."
| def __init__( | ||
| self, dims: Iterable[Union[Axes, str]], func: Union[str, Callable] = 'max', | ||
| clip_method: Clip = Clip.CLIP | ||
| self, |
There was a problem hiding this comment.
I'm confused about what is this the init method for?
There was a problem hiding this comment.
This is the init for the Reduce class. There is an init method for the FunctionSource class starting at L103.
| Make a test ImageStack | ||
|
|
||
| ''' | ||
| """Make a test ImageStack.""" | ||
|
|
||
| # Make the test image | ||
| test = np.ones((2, 4, 1, 2, 2), dtype='float32') * 0.1 |
There was a problem hiding this comment.
could we use imagestack_factory() here instead? Or maybe add a imagestack_from_numpy method to factories/all_purpose.py
There was a problem hiding this comment.
could we use imagestack_factory() here instead?
Plausibly, though the inputs and outputs are very precise and not very numerous. Not sure if it makes sense to.
imagestack_from_numpy
ImageStack.from_numpy is basically that, isn't it?
ttung
force-pushed
the
tonytung-reduce
branch
2 times, most recently
from
0347021 to
b44147e
Compare
Module defaults to np (== numpy). Class no longer accepts a callable. Alias list is a dict that relatively easy to extend. Test plan: travis
| @@ -17,53 +22,141 @@ | |||
|
|
|||
| class Reduce(FilterAlgorithmBase): | |||
There was a problem hiding this comment.
@ttung given how codebases typically mirror map & reduce, I wonder if apply should be exposed in a similar fashion.
There was a problem hiding this comment.
Yes, I agree. I thought there was already an issue for that, but maybe I'm wrong.
It's also possible that Reduce might work for this already.
Module defaults to np (== numpy). Class no longer accepts a callable. Alias list is a dict that relatively easy to extend.
Test plan: travis