fix clip bug in resize when anti_aliasing is applied (#5202) #5209

abouysso · 2021-01-29T09:42:20Z

Description

Clipping was performed based on the values of the filtered image and not
the input image, which can produce values outside of input values range.

Added clarification on kwarg anti_aliasing description: it is applied by
default for non bool type inputs when downsizing.

Closes #5202

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.

clipping was performed based on the values of the filtered image and not the input image, which can produce values outside of input values range. Added clarification on kwarg anti_aliasing description: it is applied by default for non bool type inputs when downsizing.

rfezzani

Thank you @abouysso, I think that the 2D case is not fixed with the proposed approach :(
Can you please also add a test?

rfezzani · 2021-01-29T10:06:25Z

skimage/transform/_warps.py

@@ -172,12 +174,12 @@ def resize(image, output_shape, order=None, mode='reflect', cval=0, clip=True,
        tform.params[0, 1] = 0
        tform.params[1, 0] = 0

-        out = warp(image, tform, output_shape=output_shape, order=order,
+        out = warp(img_in, tform, output_shape=output_shape, order=order,


The proposed solution doesn't work in this case since the clipping is made in warp 😕

Thanks for pointing this out ! Fixed it in fallback code by performing clipping outside of warp function 😃.

Excellent idea! The _clip_warp_output call can be performed outside the if ... elif ... else ... block once for all now 😉

skimage/transform/_warps.py

Also removed unnecessary copy of the input data.

skimage/transform/_warps.py

abouysso · 2021-01-29T18:01:52Z

I ran in another issue, also related to potential out of input range values produced by ndi.gaussian_filter.
When kwarg order is set to 0 (nearest neighbors interpolation), no clipping is performed since such interpolation does not produce out of range data. Thus it seems to me that _clip_warp_output behavior needs to be modified in order to be able to handle such cases.

I see two ways of doing so:

perform clipping as long as clip is True, even if order is 0.
add anti_aliasing as boolean kwarg to the _clip_warp_output function, and perform clipping if clip is True and either order != 0 or anti_aliasing

This allows _clip_warp_output to clip out of input range values when caused by gaussian filter and not interpolation function.

pep8speaks · 2021-01-30T17:26:25Z

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

In the file skimage/transform/_warps.py:

Line 152:49: W504 line break after binary operator

In the file skimage/transform/tests/test_warps.py:

Line 385:54: E225 missing whitespace around operator

Comment last updated at 2021-11-21 23:29:06 UTC

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

rfezzani · 2021-02-01T09:21:29Z

skimage/transform/_warps.py

-               np.asarray(output_shape, dtype=float))
+    factors = np.divide(input_shape, output_shape)
+    # create copy so input values range stays accessible through image for clip
+    img_in = image


I see a case where this approach doesn't work: when image dtype is not floating point... In the case of uint8 for example, values will not be clipped between 0 and 1 as it is supposed to be...
We must call convert_to_float here to fix this!

Indeed there could still be an issue when preserve_range is False 😄 Otherwise values will be clipped between 0 and 255 as expected. I will correct this as you suggested !

Also, personally I dislike reassigning values in the middle of a function, so, rather than copy image to image_in then overwriting image, how about just renaming the downstream images? ie below, use image_smoothed = ndi.gaussian_filter(image, ...). What do you think @abouysso?

That's in fact an option, but you need to define image_smoothed even when anti_aliasing is False in this case

I instead created img_bounds to save input image bounds for later clipping... Though?

Clipping is performed w.r.t the values of image. In case preserve_range is False, for uint8 data for example, the reference can be outdated and produce unexpected behavior of clipping. Because data types checks are performed, a new variable input_type is defined to store input dtype.

skimage/transform/_warps.py

skimage/transform/tests/test_warps.py

Test is performed with preserve_range True and False

skimage/transform/_warps.py

rfezzani · 2021-02-01T18:03:53Z

You forgot to import pytest, otherwise it looks really good! Thank you @abouysso 👌

rfezzani

Thank you @abouysso, it looks good! @scikit-image/core, the PR is ready for review 😉. CI failures seems unrelated.

stefanv

Thanks for the patch, @abouysso!

@rfezzani's review comments look good, and I left one additional one.

skimage/transform/_warps.py

rfezzani · 2021-04-13T07:59:17Z

@scikit-image/core, can anyone please find some time to review this, thanks 😉

skimage/transform/_warps.py

down-sizing now used consistently in resize function, so its clearer than previous mix between down-sizing, down-sampling and down-scaling.

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

…mage into fix-issue-5202

mkcor · 2021-04-19T11:52:47Z

Hi @abouysso,

Merging #5327 has led to a conflict in file skimage/transform/_warps.py for this PR. Are you comfortable resolving it (either locally in your file editor or in GitHub's online editor)?

…nto fix-issue-5202

abouysso · 2021-04-20T08:24:20Z

Hi @abouysso,

Merging #5327 has led to a conflict in file skimage/transform/_warps.py for this PR. Are you comfortable resolving it (either locally in your file editor or in GitHub's online editor)?

Thanks for pointing this out ! I'm working on it, conflict is solved locally, but new errors are raised on pytest. A quick look seems to indicate those errors comes from test functions modified in #5327 . I still have to find the issue 😄

skimage/transform/_warps.py

…ge#5327

This allows to give the expected dtype as input of ndi.gaussian_filter when anti-aliasing is performed

jni

@abouysso apologies that it has taken so long to to review this... Our team members have been rather overwhelmed this year. I've left a couple of comments but this should be ready very quickly! Do you have the time/inclination to look at my comments, or would you rather we take over from here?

Thank you!

skimage/transform/_warps.py

jni · 2021-05-12T05:24:35Z

skimage/transform/_warps.py

@@ -133,17 +139,13 @@ def resize(image, output_shape, order=None, mode='reflect', cval=0, clip=True,
                warn("Anti-aliasing standard deviation greater than zero but "
                     "not down-sampling along all axes")
        image = ndi.gaussian_filter(image, anti_aliasing_sigma,
-                                    cval=cval, mode=ndi_mode)
+                                     cval=cval, mode=ndi_mode)


Suggested change

cval=cval, mode=ndi_mode)

cval=cval, mode=ndi_mode)

jni · 2021-05-12T05:26:25Z

skimage/transform/_warps.py

-               np.asarray(output_shape, dtype=float))
+    factors = np.divide(input_shape, output_shape)
+    # create copy so input values range stays accessible through image for clip
+    img_in = image


Also, personally I dislike reassigning values in the middle of a function, so, rather than copy image to image_in then overwriting image, how about just renaming the downstream images? ie below, use image_smoothed = ndi.gaussian_filter(image, ...). What do you think @abouysso?

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

rfezzani · 2021-10-25T13:28:30Z

@abouysso, I resolved the merge conflicts. Can you please address @jni comments if you have any bandwidth for this 😉

skimage/transform/_warps.py

rfezzani · 2021-11-18T14:02:35Z

@scikit-image/core, I added this PR to 0.19 milestone because it is close and think it fixes skimage.transform.resize which is a sensible function.
@abouysso, do you have bandwidth to work on this soon, otherwise I can take over if you wish 😉

rfezzani · 2021-11-22T08:21:42Z

skimage/transform/_warps.py

@@ -220,7 +220,6 @@ def resize(image, output_shape, order=None, mode='reflect', cval=0, clip=True,
                   preserve_range=preserve_range)

    else:  # n-dimensional interpolation
-        order = _validate_interpolation_order(input_type, order)


Good catch @grlee77

rfezzani · 2021-11-22T08:22:06Z

🎉 Thank you @grlee77

rfezzani reviewed Jan 29, 2021

View reviewed changes

rfezzani added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Jan 29, 2021

fixed clip bug in resize for 2D interpolation in fallback code.

986afa1

Also removed unnecessary copy of the input data.

rfezzani reviewed Jan 29, 2021

View reviewed changes

skimage/transform/_warps.py Outdated Show resolved Hide resolved

abouysso marked this pull request as draft January 29, 2021 15:23

abouysso added 2 commits January 29, 2021 19:21

add anti_aliasing optional kwarg to _clip_warp_output

4eeebfa

This allows _clip_warp_output to clip out of input range values when caused by gaussian filter and not interpolation function.

test if clipping in resize works as expected with anti_aliasing=True

7d5aef3

abouysso and others added 4 commits January 30, 2021 18:28

_clip_ward_output only called once outside of if block

f792229

line too long fixed in _clip_warp_output description to comply with PEP8

02851c6

Update resize doc with clearer behavior of anti_aliasing

4c7904a

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

line too long fixed in resize description to comply with PEP8

e30de63

abouysso marked this pull request as ready for review January 30, 2021 17:45

rfezzani reviewed Feb 1, 2021

View reviewed changes

skimage/transform/_warps.py Outdated Show resolved Hide resolved

rfezzani reviewed Feb 1, 2021

View reviewed changes

skimage/transform/tests/test_warps.py Outdated Show resolved Hide resolved

abouysso added 2 commits February 1, 2021 15:32

test clipping in resize for float64 and uint8 data

b04f6a7

Test is performed with preserve_range True and False

fixed way to import the resize function in test_warps

cb63c58

rfezzani reviewed Feb 1, 2021

View reviewed changes

skimage/transform/_warps.py Outdated Show resolved Hide resolved

test of clip in resized updated with pytest parametrize

149d4d0

import pytest in test_warps

97d3d26

rfezzani approved these changes Feb 1, 2021

View reviewed changes

rfezzani added the action: mrg+1 label Feb 1, 2021

alexdesiqueira mentioned this pull request Feb 1, 2021

2021's calendar of community management #5169

Closed

stefanv reviewed Feb 2, 2021

View reviewed changes

skimage/transform/_warps.py Outdated Show resolved Hide resolved

anti-aliasing True if dtype not bool and downsizing at least along 1 dim

9701c13

mkcor reviewed Apr 13, 2021

View reviewed changes

abouysso and others added 5 commits April 14, 2021 09:55

fixed typo in test_warps funtion name

e9f0e8a

rewording anti_aliasing kwarg description

c88c036

rewording: term down-sizing used consistently in resize docstring

a83b5fe

down-sizing now used consistently in resize function, so its clearer than previous mix between down-sizing, down-sampling and down-scaling.

Apply suggestions from code review

2f689b1

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

Merge branch 'fix-issue-5202' of https://github.com/abouysso/scikit-i…

b465135

…mage into fix-issue-5202

Merge branch 'main' of https://github.com/scikit-image/scikit-image i…

5f55248

…nto fix-issue-5202

rfezzani reviewed Apr 20, 2021

View reviewed changes

skimage/transform/_warps.py Outdated Show resolved Hide resolved

abouysso added 2 commits April 20, 2021 11:08

update test function to adapt expected behavior defined in scikit-ima…

5b25276

…ge#5327

order validation and eventual conversion to float is performed earlier

2816e24

This allows to give the expected dtype as input of ndi.gaussian_filter when anti-aliasing is performed

jni reviewed May 12, 2021

View reviewed changes

rfezzani and others added 5 commits May 21, 2021 09:55

Update skimage/transform/_warps.py

7191ad2

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

Update skimage/transform/_warps.py

85d7a47

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

Update skimage/transform/_warps.py

fe41d96

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

Update skimage/transform/_warps.py

6a1dc3e

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

Merge branch 'main' into fix-issue-5202

5d5034f

rfezzani reviewed Oct 25, 2021

View reviewed changes

skimage/transform/_warps.py Outdated Show resolved Hide resolved

Update skimage/transform/_warps.py

466386f

rfezzani added this to the 0.19 milestone Nov 18, 2021

Riadh and others added 3 commits November 18, 2021 17:15

Save input bounds for clip instead of input copy

ed06e2b

Refactor img_bounds assignement to one line

ea8edf4

remove duplicate call to validate_interpolation_order

67c32f6

grlee77 merged commit 8f3f49e into scikit-image:main Nov 22, 2021

rfezzani reviewed Nov 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix clip bug in resize when anti_aliasing is applied (#5202) #5209

fix clip bug in resize when anti_aliasing is applied (#5202) #5209

abouysso commented Jan 29, 2021 •

edited

rfezzani left a comment

rfezzani Jan 29, 2021

abouysso Jan 29, 2021

rfezzani Jan 29, 2021

abouysso commented Jan 29, 2021 •

edited

pep8speaks commented Jan 30, 2021 •

edited

rfezzani Feb 1, 2021

abouysso Feb 1, 2021

jni May 12, 2021

rfezzani May 21, 2021

rfezzani Nov 18, 2021

rfezzani commented Feb 1, 2021

rfezzani left a comment

stefanv left a comment

rfezzani commented Apr 13, 2021

mkcor commented Apr 19, 2021

abouysso commented Apr 20, 2021

jni left a comment

jni May 12, 2021

jni May 12, 2021

rfezzani commented Oct 25, 2021

rfezzani commented Nov 18, 2021 •

edited

rfezzani Nov 22, 2021

rfezzani commented Nov 22, 2021

fix clip bug in resize when anti_aliasing is applied (#5202) #5209

fix clip bug in resize when anti_aliasing is applied (#5202) #5209

Conversation

abouysso commented Jan 29, 2021 • edited

Description

Checklist

For reviewers

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

abouysso commented Jan 29, 2021 • edited

pep8speaks commented Jan 30, 2021 • edited

Comment last updated at 2021-11-21 23:29:06 UTC

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 Feb 1, 2021

rfezzani left a comment

Choose a reason for hiding this comment

stefanv left a comment

Choose a reason for hiding this comment

rfezzani commented Apr 13, 2021

mkcor commented Apr 19, 2021

abouysso commented Apr 20, 2021

jni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfezzani commented Oct 25, 2021

rfezzani commented Nov 18, 2021 • edited

Choose a reason for hiding this comment

rfezzani commented Nov 22, 2021

abouysso commented Jan 29, 2021 •

edited

abouysso commented Jan 29, 2021 •

edited

pep8speaks commented Jan 30, 2021 •

edited

rfezzani commented Nov 18, 2021 •

edited