Clip output in denoise_tv_bregman

[jni]

This is pretty simple: always clip the output of denoise_tv_bregman in order to prevent invalid output due to numerical errors. Fixes #1939. However, the tests are failing in 2.7 and 3.5. Anyone got any idea why???

[ahojnnes]

I don't think this is the right solution, the clipping should be optional. There is nothing in the TV denoising equation that guarantees that the input range is not altered.

[ahojnnes]

Optional in the sense that there should be a parameter that would default to true.

[ahojnnes]

There is a reason this variable is called u. It refers to a variable in the equation further above.

[jni]

I don't think that's very clear... I would suggest that we keep out or denoised (both are used in the file) and replace u in the equation with that name. What do you think?

[ahojnnes]

@jni Sounds good!

[jni]

@ahojnnes sure, I can make it optional, but it still doesn't help me figure out why np.clip isn't doing its job in the tests! Any ideas?

[jni]

Or why there's no Travis???? =O

[emmanuelle]

@jni this is so weird... Hope Travis will reappear on your next commit with clipping being optional :-)

[jni]

@ahojnnes (pinging you because you seemed most familiar with this algorithm, correct me if wrong) so it looks like the failures are due to sometimes producing nan output for the test input. I added a print(np.max(out)) statement before the assertion, and then ran the tests three times in a row. Nothing changed between the runs at all:

(skimtest)
nuneziglesiasj@aradyne Mon Feb 22 16:23
 ~/projects/scikit-image/skimage/restoration/tests (denoise-clip)$ nosetests test_denoise:test_denoise_tv_bregman_approx_errors
.
----------------------------------------------------------------------
Ran 1 test in 0.002s

OK
(skimtest)
nuneziglesiasj@aradyne Mon Feb 22 16:24
 ~/projects/scikit-image/skimage/restoration/tests (denoise-clip)$ nosetests test_denoise:test_denoise_tv_bregman_approx_errors
.
----------------------------------------------------------------------
Ran 1 test in 0.001s

OK
(skimtest)
nuneziglesiasj@aradyne Mon Feb 22 16:24
 ~/projects/scikit-image/skimage/restoration/tests (denoise-clip)$ nosetests test_denoise:test_denoise_tv_bregman_approx_errors
F
======================================================================
FAIL: Ensure output is valid image when valid input image is given.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/nuneziglesiasj/anaconda/envs/skimtest/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/Volumes/Projects/scikit-image/skimage/restoration/tests/test_denoise.py", line 172, in test_denoise_tv_bregman_approx_errors
    assert np.max(out) <= 1
AssertionError:
-------------------- >> begin captured stdout << ---------------------
nan

... ??? Any ideas?

[ahojnnes]

I will check later today.

[ahojnnes]

Hm... there is no randomness in the algorithm. The only reason I can image is uninitialized memory, e.g., here https://github.com/jni/scikit-image/blob/denoise-clip/skimage/restoration/_denoise_cy.pyx#L173 ?

[jni]

@ahojnnes that was a good catch! But it doesn't explain it — each of those variables is later initialised before use. Unless I'm missing something... Which obviously I am... =\

[ahojnnes]

@jni Let's finally solve this nasty issue. I think, I found the problem: there is an out of bounds access and the loop should be for r in range(1, rows): / for c in range(1, cols): and not rows + 1 / cols + 1. Can you try?

[jni]

@ahojnnes 😱 😱 😱 Well-spotted! And it seems to have fixed it!

[ahojnnes]

@jni Hm. The tests still seem to fail in all environments.

[jni]

Hmm worked on my laptop. Will investigate soon, can't right now...

[ahojnnes]

@jni Gentle ping :-)

[jni]

I'm on holiday till the 5th, thought I could catch a few mins for this but now looking unlikely... Will revisit next week.

[ahojnnes]

LGTM, if the test case is fixed.

[jni]

@ahojnnes oh wow, LOL, just saw the test failure, I wonder if the out of range access was causing most of the range problems anyway! Working on this...

[ahojnnes]

@jni Ping :-)

[jni]

@ahojnnes I've had this tab open for the past few days. I'm not sure how to proceed now that the original "failure" passes... I was going to try to find another failing case, but that might be hard, so it's in the back burner... Thoughts?

[alexdesiqueira]

@jni how are we on this one? :)

[pep8speaks]

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-28 23:19:56 UTC

[jni]

@alexandrejaguar thanks for the ping and all the other triaging you've been doing, it's great! Let's see how this rebase turns out... =)

[alexdesiqueira]

@jni glad I could help!

[rfezzani]

This line is causing the CI failures: u is not declared and cu is not used afterward...
Simply removing this line may solve the problem.

[rfezzani]

@jni Is this requirement (clipping denoise_tv_bregman output) still up to date?

[grlee77]

@jni Is this requirement (clipping denoise_tv_bregman output) still up to date?

I think it should only clip by default if the input is not floating point to make it consistent with denoise_wavelet. That function used to always clip the output, but was modified to only do so for non-float inputs since img_as_float no longer rescales to [0, 1] (or [-1, 1]):

    # floating-point inputs are not rescaled, so don't clip their output.
    clip_output = image.dtype.kind != 'f'

If you want to have an explicit clip argument, then I would set the default to None and then set to True only for non-float inputs, as done above.