Simplify resize implementation using new SciPy 1.6 zoom option #5173

grlee77 · 2021-01-05T13:30:03Z

Description

This PR greatly simplifies the implementation of skimage.transform.resize, by using SciPy 1.6.0's new grid_mode=True option to scipy.ndimage.zoom. This allows us to avoid manually generating coordinates and calling map_coordinates.

numpy.pad -> scipy.ndimage mode conversion was also updated for SciPy 1.6.
closes gh-5064

Currently this new code path replaces both the scipy.ndimage.map_coordinates code path as well as the one relying on scikit-image's own Cython-based interpolation code. We could also potentially leave the existing Cython code path in place (it was ~10-20% faster than ndimage.zoom in 2D with orders 0 or 1). Let me know which is preferred.

It can be seen in the benchmarks below that map_coordinates is occasionally faster than zoom (see e.g. the 3D case for order=5), but this comes at cost of substantially higher memory use needed to store the full set of output coordinates.

ASV benchmarks for resize and rescale (which just calls resize internally) were added. The following table summarizes some results for this PR vs. v0.18.1 in both run time and memory usage. This PR is sometimes substantially faster (3D for order 0, 1) and occasionally a bit slower.

shape	order	v0.18.1	this PR	memory v0.18.1	memory PR
(2000, 4000)	0	62.7 ms	79.6 ms	160	157
(2000, 4000)	1	160 ms	177 ms	160	158
(2000, 4000)	3	531 ms	479 ms	160	166
(2000, 4000)	5	1.48 s	1.04 s	637	166
(150, 150, 150)	0	153 ms	32.8 ms	255	120
(150, 150, 150)	1	230 ms	126 ms	255	120
(150, 150, 150)	3	828 ms	896 ms	255	129
(150, 150, 150)	5	2.44 s	3.04 s	255	129

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

For reviewers

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.

…-image#5164

- Round trips start with stains, such that the roundtrip does not require negative values. - Using a blue+red+orange stain combination, the test previously used a 2-stain combination that made it impossible to do a correct roundtrip.

use grid-constant and grid-wrap for SciPy >= 1.6.0 closes scikit-imagegh-5064

pep8speaks · 2021-01-05T13:30:12Z

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file skimage/metrics/simple_metrics.py:

Line 189:21: E226 missing whitespace around arithmetic operator
Line 252:13: E126 continuation line over-indented for hanging indent
Line 255:13: E123 closing bracket does not match indentation of opening bracket's line

In the file skimage/registration/tests/test_tvl1.py:

Line 31:31: E226 missing whitespace around arithmetic operator

In the file skimage/transform/_warps.py:

Line 150:60: W504 line break after binary operator

Comment last updated at 2021-01-22 02:51:54 UTC

emmanuelle · 2021-01-05T21:40:21Z

transform/tests/test_warps.py::test_downsize_anti_aliasing seems to be failing consistently.

emmanuelle · 2021-01-05T21:51:01Z

Maybe an item should be added in the TODO so that we don't forget to remove the other code when the scipy dep is bumped to 1.6 (this will probably take some time... so a reminder might be good).

Improvement to the IHC example. Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

grlee77 · 2021-01-05T23:14:29Z

transform/tests/test_warps.py::test_downsize_anti_aliasing seems to be failing consistently.

I was relying on SciPy to raise on unrecognized modes, but SciPy raises a RuntimeError rather than the ValueError the test was expecting. I added a ValueError again here for consistency with prior behavior, but if you don't think it is important we can just remove that error check from the tests.

jni · 2021-01-06T00:12:24Z

TODO.txt

+* When ``scipy`` is set to >= 1.16, remove legacy (i.e. non-zoom) code paths in
+``skimage.transform.resize``.


Thanks to NEP-29, we can now do date-based reminders (🎉!): this will be done on Jan 1 2022. Should we add a separate time-based section to the todos?

good idea, yes

Oops — that should be 2023. Time flies when you're having fun! =P

See how this latest commit reorganizing the "Other" section looks.

We can actually do many of the 2021 items now (we already require NumPy 1.16 and we SciPy 1.1 was released in May 2018).

jni · 2021-01-06T00:16:05Z

Love it! I think the few specific cases where performance is worse, the tradeoff is worth it for code simplicity.

Stain unmixing fix

…cikit-image#5175) This should avoid stochastic test failures that have been occuring on GitHub Actions

@mkcor

* Add normalized mutual information metric * Add tests for NMI * Add NMI to metrics init * Fix error message for incorrect target shape * Add 3D NMI test * properly add nmi to metrics init * _true, _test -> 0, 1 * ravel -> reshape(-1) * Apply suggestions from @mkcor's code review Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org> * Fix warning and exception messages Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

based on NEP-29 24 month window for dropping upstream package version

mode 'nearest' is a scipy.ndimage mode, but warp only accepts the numpy.pad equivalent of 'edge'

Right now the data is not sampled every half degree. Also, -pi/2 to pi/2.

…5182) * Correction to linspace call in hough_line default `theta = np.pi / 2 - np.arange(180) / 180.0 * np.pi` done correctly below. * BUG: Fixup to image orientation in example * DOC: Tiny typo * Changed range to conform to documentation The original was [pi/2, -pi/2), not [-pi/2, pi/2). Updated wording in docs. * Fixed x-bounds of hough image in example * Fixed long line * Fixes to tests Number of lines in checkerboard, longer line in peak ordering assertion, updated angle * Accidentally modified the wrong value. * Updated example to include three lines As requested in review

…image#5183)

* Replace correlate_sparse Cython version with a simple slicing-based variant in _mean_std This slicing variant has two benefits: 1.) profiled 30-40% faster on 2d and 3d cases I tested with 2.) can immediately be used by other array backends such as CuPy * remove intermediate padded_sq variable to reduce memory footprint * Update skimage/filters/_sparse.py remove duplicate declarations Co-authored-by: Riadh Fezzani <rfezzani@gmail.com> * move _to_np_mode and _to_ndimage_mode to _shared.utils.py * add comment about corner_index needing to be present * raise RuntimeError on missing corner index * update code style based on reviewer comments Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

…ikit-image#5187) Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

rfezzani

Thank you @grlee77, I added some suggestions/remarks, but the PR is good as is 😉

rfezzani · 2021-01-20T14:14:53Z

skimage/registration/_optical_flow.py

@@ -76,7 +76,7 @@ def _tvl1(reference_image, moving_image, flow0, attachment, tightness,
                                             [1] + reference_image.ndim * [3])

        image1_warp = warp(moving_image, get_warp_points(grid, flow_current),
-                           mode='nearest')


👀 Was this a bug?

I think it was an unintentional use of the scipy.ndimage name rather than letting the function convert from the numpy.pad equivalent name ('edge'). The docstring to warp conforms to numpy.pad style names (i.e. 'edge' rather than its scipy.ndimage equivalent: 'nearest'). It just so happened that warp didn't raise an error on unrecognized modes and just passed them along unmodified.

May be something to investigate 😕...

skimage/transform/_warps.py

rfezzani · 2021-01-20T14:28:33Z

skimage/transform/_geometric.py

@@ -6,15 +6,6 @@
 from .._shared.utils import get_bound_method_class, safe_as_int


-def _to_ndimage_mode(mode):


I see in master branch that _to_ndimage_mode is not used in this file but we are reimporting it from skimage._shared.utils 😖

is this just a comment or are you recommending something here? (I didn't quite follow where we are "reimporting it from")

It was just a comment 🙂

skimage/_shared/utils.py

* Change dtype of CircleModel and EllipseModel to float, to avoid integer overflows * remove failing test and add code review notes * Apply suggestions from code review Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com> * Remove unused pytest import Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com> Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com> Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

rfezzani · 2021-01-21T09:25:48Z

@grlee77, it looks like a rebase is necessary :)

…mage#5191) Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org> Co-authored-by: Riadh <r.fezzani@vitadx.com>

use grid-constant and grid-wrap for SciPy >= 1.6.0 closes scikit-imagegh-5064

based on NEP-29 24 month window for dropping upstream package version

mode 'nearest' is a scipy.ndimage mode, but warp only accepts the numpy.pad equivalent of 'edge'

Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

reuse implementation from scikit-imagegh-5173

grlee77 and others added 6 commits January 3, 2021 05:24

resize using scipy.ndimage.zoom

917fe5c

Clip negative values in output of RGB to HED conversion. Fixes scikit…

94883e2

…-image#5164

Fixed stain separation tests.

ff18827

- Round trips start with stains, such that the roundtrip does not require negative values. - Using a blue+red+orange stain combination, the test previously used a 2-stain combination that made it impossible to do a correct roundtrip.

Improve IHC stain separation example.

be5e144

BENCH: add benchmarks for resize and rescale

37e64b0

move mode conversion code to _shared.utils.py

5ab53fd

use grid-constant and grid-wrap for SciPy >= 1.6.0 closes scikit-imagegh-5064

grlee77 added 🔧 type: Maintenance Refactoring and maintenance of internals 📈 type: Performance labels Jan 5, 2021

separate grid mode conversion utility

db70653

crisluengo and others added 4 commits January 5, 2021 15:15

Update doc/examples/color_exposure/plot_ihc_color_separation.py

e03c4ef

Improvement to the IHC example. Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

add TODO for removing legacy code paths

4fe2e31

avoid pep8 warning in benchmark_interpolation.py

4d0540f

keep same ValueError on unrecognized modes

0dca83c

grlee77 mentioned this pull request Jan 5, 2021

fast, non-Cython implementation for correlate_sparse #5171

Merged

jni reviewed Jan 6, 2021

View reviewed changes

Alexandre de Siqueira and others added 4 commits January 5, 2021 22:18

Merge pull request scikit-image#5172 from crisluengo/stain-unmixing

84e0fe7

Stain unmixing fix

test_rank.py: fetch reference data once into a dictionary of arrays (s…

81dd9d1

…cikit-image#5175) This should avoid stochastic test failures that have been occuring on GitHub Actions

segment Other section of TODO by date

0777ae7

based on NEP-29 24 month window for dropping upstream package version

grlee77 mentioned this pull request Jan 7, 2021

update boundary mode in profile_line #5174

Merged

grlee77 added 2 commits January 7, 2021 16:07

Fix bug in optical flow code and examples

36384a1

mode 'nearest' is a scipy.ndimage mode, but warp only accepts the numpy.pad equivalent of 'edge'

fix additional cases of warp with 'nearest'->'edge'

d2e2c32

grlee77 mentioned this pull request Jan 11, 2021

2021's calendar of community management #5169

Closed

madphysicist added 2 commits January 11, 2021 22:17

DOC: Fixup to linspace in example (scikit-image#5181)

a0dd0aa

Right now the data is not sampled every half degree. Also, -pi/2 to pi/2.

madphysicist and others added 4 commits January 15, 2021 08:04

Fixed off-by one error in pixel bins in hough line transform (scikit-…

4f93a9f

…image#5183)

Added 1/2 pixel bounds to extent of displayed images (scikit-image#5184)

7cd406d

Add release step on github to RELEASE.txt (See scikit-image#5185) (sc…

fca9f16

…ikit-image#5187) Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

jni approved these changes Jan 19, 2021

View reviewed changes

rfezzani approved these changes Jan 20, 2021

View reviewed changes

mark-boer and others added 2 commits January 20, 2021 18:35

Add saturation parameter to color.label2rgb (scikit-image#5156)

8acad22

rfezzani and others added 13 commits January 22, 2021 09:30

Remove reference to opencv in threshold_local documentation (scikit-i…

2829ca5

…mage#5191) Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org> Co-authored-by: Riadh <r.fezzani@vitadx.com>

resize using scipy.ndimage.zoom

2d6c6fb

BENCH: add benchmarks for resize and rescale

e1a0275

move mode conversion code to _shared.utils.py

b01bbd0

use grid-constant and grid-wrap for SciPy >= 1.6.0 closes scikit-imagegh-5064

separate grid mode conversion utility

1ee2476

add TODO for removing legacy code paths

6610b8a

avoid pep8 warning in benchmark_interpolation.py

52abc29

keep same ValueError on unrecognized modes

2fe298b

segment Other section of TODO by date

955712c

based on NEP-29 24 month window for dropping upstream package version

Fix bug in optical flow code and examples

2d813d6

mode 'nearest' is a scipy.ndimage mode, but warp only accepts the numpy.pad equivalent of 'edge'

fix additional cases of warp with 'nearest'->'edge'

cb77e6d

Apply suggestions from code review

ddb9c22

Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

fix merge conflicts

9cce15e

grlee77 added a commit to grlee77/scikit-image that referenced this pull request Jan 22, 2021

fix _shared/utils.py

b771acd

reuse implementation from scikit-imagegh-5173

rfezzani merged commit d2fb610 into scikit-image:master Jan 22, 2021

grlee77 deleted the resize_using_zoom branch July 8, 2021 20:46

lagru mentioned this pull request Aug 23, 2022

Nearest neighbor resize of integer array changes the unique integer values #6467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify resize implementation using new SciPy 1.6 zoom option #5173

Simplify resize implementation using new SciPy 1.6 zoom option #5173

grlee77 commented Jan 5, 2021

pep8speaks commented Jan 5, 2021 •

edited

emmanuelle commented Jan 5, 2021

emmanuelle commented Jan 5, 2021

grlee77 commented Jan 5, 2021

jni Jan 6, 2021

sciunto Jan 7, 2021

jni Jan 7, 2021

grlee77 Jan 7, 2021

jni commented Jan 6, 2021

rfezzani left a comment

rfezzani Jan 20, 2021

grlee77 Jan 22, 2021

rfezzani Jan 22, 2021

rfezzani Jan 20, 2021

grlee77 Jan 22, 2021

rfezzani Jan 22, 2021

rfezzani commented Jan 21, 2021

		* When ``scipy`` is set to >= 1.16, remove legacy (i.e. non-zoom) code paths in
		``skimage.transform.resize``.

		@@ -6,15 +6,6 @@
		from .._shared.utils import get_bound_method_class, safe_as_int


		def _to_ndimage_mode(mode):

Simplify resize implementation using new SciPy 1.6 zoom option #5173

Simplify resize implementation using new SciPy 1.6 zoom option #5173

Conversation

grlee77 commented Jan 5, 2021

Description

Checklist

For reviewers

pep8speaks commented Jan 5, 2021 • edited

Comment last updated at 2021-01-22 02:51:54 UTC

emmanuelle commented Jan 5, 2021

emmanuelle commented Jan 5, 2021

grlee77 commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jni commented Jan 6, 2021

rfezzani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfezzani commented Jan 21, 2021

pep8speaks commented Jan 5, 2021 •

edited