Fit output in warp function #2740
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2740 +/- ##
==========================================
+ Coverage 86.01% 86.02% +<.01%
==========================================
Files 332 332
Lines 26627 26643 +16
==========================================
+ Hits 22904 22919 +15
- Misses 3723 3724 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Although everything appears to be in order with regards to documentation and PEP8 conventions, it would be helpful if you filled out the template provided in the future.
As with your requested changes, the resizing operation that you added only applies to 2-D images while its parent function can take in 2- or 3-D images. You should also add a test in skimage/transform/tests/test_warps.py for this enhancement, accounting for both 2-D and 3-D images.
| @@ -778,6 +760,32 @@ def warp(image, inverse_map, map_args={}, output_shape=None, order=1, | |||
|
|
|||
| if matrix is not None: | |||
| matrix = matrix.astype(np.double) | |||
|
|
|||
| if resize: | |||
| rows, cols = input_shape[:2] | |||
There was a problem hiding this comment.
This resizing operation appears to only apply to 2-D images whereas the warp function accepts both 2-D and 3-D images. Passing in a 3-D image with the argument resize=True would likely result in an output with the 3rd axis not resized properly.
There was a problem hiding this comment.
Correct; our previous code was limited this way, and can (should) be extended.
There was a problem hiding this comment.
I'm not sure what is 3-D images. It's something like arrays which have shape (512, 512, 512)? If so, how can I detect the input image is 3-D or multichannel? RGB image is (512, 512, 3) but it's not 3-D image, right?
skimage/transform/_warps.py
Outdated
| @@ -665,6 +643,10 @@ def warp(image, inverse_map, map_args={}, output_shape=None, order=1, | |||
| preserve_range : bool, optional | |||
| Whether to keep the original range of values. Otherwise, the input | |||
| image is converted according to the conventions of `img_as_float`. | |||
| resize : bool, optional | |||
There was a problem hiding this comment.
I wonder if resize (the keyword we chose before) is the correct one: it does not include some notion of the translation needed to make the transform fit. Maybe fit_output or extend_to_fit?
skimage/transform/_warps.py
Outdated
| @@ -665,6 +643,10 @@ def warp(image, inverse_map, map_args={}, output_shape=None, order=1, | |||
| preserve_range : bool, optional | |||
| Whether to keep the original range of values. Otherwise, the input | |||
| image is converted according to the conventions of `img_as_float`. | |||
| resize : bool, optional | |||
| Determine whether the shape of the output image will be automatically | |||
| calculated, so the complete rotated image exactly fits. Default is | |||
There was a problem hiding this comment.
Warp does much more than rotation.
| @@ -778,6 +760,32 @@ def warp(image, inverse_map, map_args={}, output_shape=None, order=1, | |||
|
|
|||
| if matrix is not None: | |||
| matrix = matrix.astype(np.double) | |||
|
|
|||
| if resize: | |||
| rows, cols = input_shape[:2] | |||
There was a problem hiding this comment.
Correct; our previous code was limited this way, and can (should) be extended.
| out_cols = maxc - minc + 1 | ||
| output_shape = np.ceil((out_rows, out_cols)) | ||
|
|
||
| # fit output image in new shape |
There was a problem hiding this comment.
Please add tests for the new API.
|
Hello @wkentaro! Thanks for updating the PR.
Comment last updated on November 05, 2017 at 09:36 Hours UTC |
|
Take a look at the You have to handle the following cases:
|
|
I'm not sure how to handle the third case, do you have some suggestions? |
| x = np.zeros((5, 5), dtype=np.double) | ||
| x[1, 1] = 1 | ||
| theta = -np.pi / 4 | ||
| M = np.array([[np.cos(theta), - np.sin(theta), 0], |
There was a problem hiding this comment.
For sake of uniformity and clarity, please change this to
[[np.cos(theta), - np.sin(theta), 0],
[np.sin(theta), np.cos(theta), 4],
[0, 0, 1]]as it is in your second test case.
wkentaro
changed the title
|
What's the status on this PR? |
wkentaro
force-pushed
the
resize_warp
branch
from
3c37e72
to
cc0052e
Compare
wkentaro
force-pushed
the
resize_warp
branch
from
cc0052e
to
2006aa0
Compare
|
I have resolved the conflict. Now I think it is ready to be reviewed. Could someone please review this? |
|
Is there a problem merging this PR? |
Because I need to fit the output with using the warp function.