Fix the data scaling and clipping behavior of denoise_wavelet #3584

grlee77 · 2018-12-06T23:12:40Z

Description

This PR fixes two separate issues with data scaling in denoise_wavelet.

The changes in Allow float->float conversion of any range #3052 (see also Code audit: usage of img_as_float #3373), mean that floating point inputs will no longer be automatically rescaled to the range [0, 1] or [-1, 1]. I think this is desirable, but it means that it is no longer appropriate to clip the output to this range when the input is a float.
In cases where img_as_float does rescale the image, the same rescaling should also be applied to sigma, but this was not previously being done. This was okay for the default sigma=None as the scaling occurs prior to the internal call to estimate_sigma.

I added some test cases that check both PSNR and the output range for various dtypes. All of these test cases fail without the changes in this PR.

The previously existing tests were passing because the inputs had already been scaled via img_as_float prior to calling denoise_wavelet.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

For reviewers

(Don't remove the checklist below.)

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.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

pep8speaks · 2018-12-06T23:12:44Z

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

In the file doc/examples/filters/plot_denoise_wavelet.py:

Line 66:49: E226 missing whitespace around arithmetic operator
Line 69:49: E226 missing whitespace around arithmetic operator

In the file skimage/restoration/tests/test_denoise.py:

Line 564:18: E226 missing whitespace around arithmetic operator
Line 564:34: E226 missing whitespace around arithmetic operator
Line 566:18: E226 missing whitespace around arithmetic operator
Line 566:33: E226 missing whitespace around arithmetic operator

Comment last updated at 2019-09-16 13:15:18 UTC

grlee77 · 2018-12-06T23:16:21Z

we can probably also backport this to 0.14.x because #3052 was already backported in #3390.

jni

@grlee77 thanks for tackling this! I've left a few comments. I think some require discussion, perhaps at the dev meeting. =)

jni · 2018-12-07T00:52:49Z

skimage/restoration/_denoise.py

+            if multichannel:
+                sigma = [s * scale_factor for s in sigma]
+            else:
+                sigma *= scale_factor


I'm actually quite concerned that this is a major change in the behaviour of this function on identical inputs. We could classify it as a bug and just do it, but I'd like to have a discussion about it. My gut instinct is that we should have a deprecation cycle, sadly. @scikit-image/core?

As a separate issue, this function is getting pretty long and hard to read in one go. I suggest making a helper function, e.g. image, sigma = _scale_sigma_and_image_consistently(image, sigma, multichannel). You could even bundle in the sigma-to-list expansion.

I agree that it may warrant that. It is clearly a change in behavior, but I think will be more intuitive going forward.

estimate_sigma does not do any rescaling, so an input in range [0, 1e6] might produce a huge sigma value of 1e4 or so which would clearly give a crazy oversmoothing when applied to an image that has been rescaled to range [0, 1]!

For the current release, sigma only really works if the user computed it for an image scaled to [0, 1] or [1, -1] (or if it is left to the default of None).

Oh yes, I am absolutely not arguing that we shouldn't have the change, just whether it should be a silent/hard change or a deprecation. I think we might have many people who had their sigma set (empirically) 255 times lower than intended, and now they will find that their code does no denoising at all, and be at a loss as to why.

I think the clip_output flag to avoid clipping floating point outputs to [0, 1] or [-1, 1] should be applied now as a bug fix.

I think the automatic rescaling of sigma when the image must be rescaled is convenient and the right thing to do, but that could potentially go through a deprecation cycle for the reason you mention.

Frustrating—so close to a bug fix, but not quite! We may argue for an accelerated deprecation in this case, perhaps; i.e., that the behavior changes immediately but that old users at least get a warning? Either that, or the more conservative option of deprecating.

@stefanv I'd be on board with "accelerated deprecation" in cases of bug fixes that can affect correct running of code. @grlee77 what do you think? My main concern with this kind of warning is that it should be easy to disable.

I would lean toward "accelerated deprecation". I think it is likely not uncommon that users with floating point data would be getting bad results currently.

My main concern with this kind of warning is that it should be easy to disable.

Is there a recommended way to do this? I am only familiar with the user needing to use with warnings.catch_warnings context to disable the unwanted warnings?

I'm thinking some kind of long-lived keyword argument, rescale_sigma=None, which means True, and if set to None (default), a warning is issued, but if set to True there is no warning. If set to False you get the old behaviour but a warning that this option will go away in the future. In both the docstring and the warning, explain the issue and maybe link to this discussion.

Thoughts?

The point is that catch_warnings is super-verbose and people are unlikely to want to pollute their codebase with it.

The main problem with the long-lived kwarg is that it results in yet another deprecation in the future. I don't see a better way around it though...

@JDWarner tends to have opinions about these things... =)

I think adding a rescale_sigma=Noneargument sounds good and gives an easier way to bypass any warning if desired.

skimage/restoration/_denoise.py

skimage/restoration/tests/test_denoise.py

grlee77 · 2018-12-07T05:41:10Z

I messed up the scaling in the YCbCr case and still have to revisit that (see discussion above).

grlee77 · 2018-12-07T18:19:37Z

This is still broken in the YCbCr case. I need to add a test case with a color image to the paramtrization

grlee77 · 2019-03-12T04:39:16Z

There was one CI failure in TestRank.test_rank_filter[windowed_histogram] unrelated to this PR.

I think the primary outstanding issue is how to handle the transition to this new, more user-friendly behavior. After these changes, the user can compute sigma for whatever the current values are in image. If the image is rescaled internally, sigma gets automatically rescaled in the same way. This is much less likely to result in misuse of the function or unexpected behavior due to internal rescaling that would not be obvious to the user.

grlee77 · 2019-07-22T21:57:02Z

Oh yes, I am absolutely not arguing that we shouldn't have the change, just whether it should be a silent/hard change or a deprecation. I think we might have many people who had their sigma set (empirically) 255 times lower than intended, and now they will find that their code does no denoising at all, and be at a loss as to why.

I would really like to get this fix in for 0.16. I think Juan's comment quoted above is the only thing left to address here. As currently implemented, this is a "hard change", but I can update to go the deprecation route if that is preferred.

jni · 2019-07-22T23:03:06Z

I'm still in favour of a deprecation pathway, but could be swayed by other voices! What's your current thinking @grlee77?

skimage/restoration/_denoise.py

jni · 2019-07-29T04:22:54Z

@grlee77 as usual with sphinx errors, it's hard to tell what's wrong, but comparing your formatting with the existing ones (which work), the only thing I can spot is three spaces to indent the subsequent paragraph, rather than four. Because why not.

grlee77 · 2019-07-29T15:01:57Z

Well, 3 spaces didn't seem to work either. I will try testing a bit more locally to not tie up the CI. I did check that it appeared to render okay prior to uploading the prior commit, but perhaps there was a warning I overlooked.

grlee77 · 2019-07-29T19:36:23Z

@jni: Adding a backtick fixed the documentation build issue

There is one unrelated failure on Travis in test_rank_filter.
Two others timed out downloading something via homebrew so I tried restarting those.

jni · 2019-07-30T08:47:18Z

Woot! The rank filters failure is known, see #4052. Thanks @grlee77!

fix: remove clipping of output for floating-point inputs img_as_float no longer automatically rescales floating point inputs to [0, 1] or [-1, 1] so this clipping was no longer appropriate. TST: add unscaled floating point test case to denoise_wavelet

…tently raise a ValueError if convert2ycbcr is specified for a non-multichannel image

This default value of None is equivalent to True but will raise a DeprecationWarning Setting rescale_sigma=False restores the old behaviour (no rescaling)

adjust docstring indentation add missing backtick in denoise_wavelet docstring

grlee77 · 2019-09-16T13:57:55Z

Updated the tests to account for the rename of compare_psnr -> peak_signal_noise_ratio. Should be good to go now.

grlee77 · 2019-10-03T13:46:35Z

Any objections to merging this now so it will be in 0.16? (did not want to self-merge my own PR)

jni · 2019-10-03T14:17:33Z

@scikit-image/core anyone around to pull the trigger? =) Clearly no objections from me, @grlee77! =)

NelleV · 2019-10-08T18:18:46Z

Tagging this as critical for the release.

alexdesiqueira · 2019-10-08T18:37:31Z

Thanks for the fix, @grlee77!

NelleV · 2019-10-08T18:39:25Z

Thanks everyone for contributing and merging! Will backport this to 0.16 (aka, I'll probably just rebase…)

grlee77 added the type: bug label Dec 6, 2018

grlee77 added this to the 0.15 milestone Dec 6, 2018

jni reviewed Dec 7, 2018

View reviewed changes

grlee77 force-pushed the wavelet_scaling_fix branch from f9c2076 to 7e25ef3 Compare March 11, 2019 22:50

soupault modified the milestones: 0.15, 0.16 Apr 20, 2019

grlee77 force-pushed the wavelet_scaling_fix branch from 04a5635 to a90624f Compare July 26, 2019 21:32

jni reviewed Jul 27, 2019

View reviewed changes

skimage/restoration/_denoise.py Outdated Show resolved Hide resolved

jni reviewed Jul 27, 2019

View reviewed changes

skimage/restoration/_denoise.py Outdated Show resolved Hide resolved

jni approved these changes Jul 30, 2019

View reviewed changes

soupault force-pushed the wavelet_scaling_fix branch from 82f3b51 to 512ad89 Compare August 17, 2019 13:33

grlee77 and others added 8 commits September 16, 2019 09:12

DOC: update release notes

431ce02

style refactor based on review comments

e70cb7c

use parametrize for dtype in wavelet scaling tests

4210606

pep8 fix

f0f6bd1

Do not rescale sigma when convert2ycbcr is True

641b890

expand parametrize test cases for test_wavelet_denoising_scaling

821fa39

setting clip_output must occur prior to _scale_sigma_and_image_consis…

5642c1e

…tently raise a ValueError if convert2ycbcr is specified for a non-multichannel image

grlee77 and others added 8 commits September 16, 2019 09:12

expand test_wavelet_denoising_scaling to include 2d multichannel cases

412de30

autoscale sigmas appropriately for the YCbCr case too

a8f26ff

Add rescale_sigma argument to denoise wavelet.

51598d9

This default value of None is equivalent to True but will raise a DeprecationWarning Setting rescale_sigma=False restores the old behaviour (no rescaling)

Specify rescale_sigma=True in denoise_wavelet examples

0ce4f5c

DOC: mention rescale_sigma in the release notes

e2df245

use versionadded and versionchanged in denoise_wavelet docstring

5f29df1

adjust docstring indentation add missing backtick in denoise_wavelet docstring

compare_psnr->peak_signal_noise_ratio

2d7e2c9

remove unused import in test_denoise.py

459813b

grlee77 force-pushed the wavelet_scaling_fix branch from 512ad89 to 459813b Compare September 16, 2019 13:15

grlee77 mentioned this pull request Oct 8, 2019

DOC creating release notes for 0.16 #4217

Merged

9 tasks

NelleV added the ⚠️ Critical Reserved for newly introduced bugs in a final release label Oct 8, 2019

alexdesiqueira merged commit 19483d8 into scikit-image:master Oct 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the data scaling and clipping behavior of denoise_wavelet #3584

Fix the data scaling and clipping behavior of denoise_wavelet #3584

grlee77 commented Dec 6, 2018

pep8speaks commented Dec 6, 2018 •

edited

grlee77 commented Dec 6, 2018

jni left a comment

jni Dec 7, 2018

grlee77 Dec 7, 2018

jni Dec 7, 2018

grlee77 Mar 12, 2019

stefanv Jul 22, 2019

jni Jul 22, 2019

grlee77 Jul 23, 2019

jni Jul 23, 2019

jni Jul 23, 2019

grlee77 Jul 23, 2019

grlee77 commented Dec 7, 2018

grlee77 commented Dec 7, 2018

grlee77 commented Mar 12, 2019

grlee77 commented Jul 22, 2019

jni commented Jul 22, 2019

jni commented Jul 29, 2019

grlee77 commented Jul 29, 2019

grlee77 commented Jul 29, 2019

jni commented Jul 30, 2019

grlee77 commented Sep 16, 2019

grlee77 commented Oct 3, 2019

jni commented Oct 3, 2019

NelleV commented Oct 8, 2019

alexdesiqueira commented Oct 8, 2019

NelleV commented Oct 8, 2019

Fix the data scaling and clipping behavior of denoise_wavelet #3584

Fix the data scaling and clipping behavior of denoise_wavelet #3584

Conversation

grlee77 commented Dec 6, 2018

Description

Checklist

For reviewers

pep8speaks commented Dec 6, 2018 • edited

Comment last updated at 2019-09-16 13:15:18 UTC

grlee77 commented Dec 6, 2018

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

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

Choose a reason for hiding this comment

grlee77 commented Dec 7, 2018

grlee77 commented Dec 7, 2018

grlee77 commented Mar 12, 2019

grlee77 commented Jul 22, 2019

jni commented Jul 22, 2019

jni commented Jul 29, 2019

grlee77 commented Jul 29, 2019

grlee77 commented Jul 29, 2019

jni commented Jul 30, 2019

grlee77 commented Sep 16, 2019

grlee77 commented Oct 3, 2019

jni commented Oct 3, 2019

NelleV commented Oct 8, 2019

alexdesiqueira commented Oct 8, 2019

NelleV commented Oct 8, 2019

pep8speaks commented Dec 6, 2018 •

edited