Emit error in structural_similarity when data_range is not specified in case of floating point data. by mcourteaux · Pull Request #6612 · scikit-image/scikit-image

mcourteaux · 2022-11-04T12:12:15Z

Additionally, emit warnings when integers other than uint8 are used.

Description

Closes #6602.

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
Descriptive commit messages (see below)

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.
There is a bot to help automate backporting a PR to an older branch. For
example, to backport to v0.19.x after merging, add the following in a PR
comment: @meeseeksdev backport to v0.19.x
To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
can be removed and then added again. The benchmark output can be checked in
the "Actions" tab.

…. Emit warnings when integers other than uint8.

mcourteaux · 2022-11-04T12:52:20Z

I'm facing a broken test, due to the SSIM being used to validate the equality of total variation denoising, but due to not specifying data_range, the result was too generous. The test checks for SSIM > 0.99, but actually is between 0.98 and 0.99. I have no expert knowledge on this algorithm, nor test:

scikit-image/skimage/restoration/tests/test_denoise.py

Lines 165 to 180 in e68ff34

    
           def test_denoise_tv_chambolle_weighting(): 
        
               # make sure a specified weight gives consistent results regardless of 
        
               # the number of input image dimensions 
        
               rstate = np.random.default_rng(1234) 
        
               img2d = astro_gray.copy() 
        
               img2d += 0.15 * rstate.standard_normal(img2d.shape) 
        
               img2d = np.clip(img2d, 0, 1) 
        
               # generate 4D image by tiling 
        
               img4d = np.tile(img2d[..., None, None], (1, 1, 2, 2)) 
        
               w = 0.2 
        
               denoised_2d = restoration.denoise_tv_chambolle(img2d, weight=w) 
        
               denoised_4d = restoration.denoise_tv_chambolle(img4d, weight=w) 
        
               assert structural_similarity(denoised_2d, 
        
                                            denoised_4d[:, :, 0, 0]) > 0.99

Now I don't know if the test should pass, and the threshold should be lowered to 0.98, or this test was broken and now we discover a bug in this test/code. @ahojnnes @mkcor @grlee77: you are all authors of the lines of this test, according to git blame. Can you have a look?

… Fix a broken test that was using structural_similarity incorrectly;

mcourteaux · 2022-11-04T13:01:03Z

For now I lowered the threshold to 0.98 to just be able to run all tests with GH actions.

stefanv · 2022-11-05T03:49:13Z

Seems reasonable to me. I hope one of those authors can take a look soon, just to confirm, then we can get this in.

Thank you very much for spotting and fixing this issue, @mcourteaux!

mkcor · 2022-11-05T08:50:40Z

Hi @mcourteaux,

thank you so much for this follow-up PR!

Test test_denoise_tv_chambolle_weighting with the 0.99 threshold comes from PR #1839 (more specifically, commit 682f089), a major contribution by @grlee77 which dates back to ~7 years ago. My understanding is that the 0.99 threshold doesn't bear any specific meaning other than 'close enough to 1.0' in this test, which is checking for consistency (not accuracy). So I'm not shocked by the fact that we would need to relax this threshold to 0.98 now that you're enforcing the proper data range.

The bug was in the structural_similarity function and you're fixing it with this PR. I wouldn't say that test_denoise_tv_chambolle_weighting had a bug, it's just a weaker kind of test.

Reviewing now!

mkcor

Thank you, @mcourteaux! Your contribution is bringing such a great improvement. I've left suggestions which are essentially stylistic. Happy to approve after you've addressed them!

skimage/metrics/_structural_similarity.py

skimage/metrics/tests/test_structural_similarity.py

mkcor · 2022-11-05T09:07:52Z

skimage/metrics/_structural_similarity.py

        dmin, dmax = dtype_range[im1.dtype.type]
        data_range = dmax - dmin
+        if np.issubdtype(im1.dtype, np.integer) and (im1.dtype != np.uint8):
+            warn("Setting data_range based on im1.dtype. " +


Unrelated to this PR, but I'm just frustrated we still have im1, im2 around... cf. https://github.com/scikit-image/scikit-image/pull/4187/files#r329088276

skimage/restoration/tests/test_denoise.py

skimage/metrics/tests/test_structural_similarity.py

skimage/metrics/_structural_similarity.py

skimage/metrics/tests/test_structural_similarity.py

mcourteaux · 2022-11-08T13:31:59Z

Applied your suggestions, @mkcor. I guess the only thing left to do is the checklist for reviewers, regarding change notes etc.

mkcor

Thank you, @mcourteaux!

@lagru this is coming up for #6556 😉

mcourteaux · 2022-11-18T13:39:47Z

What's the plan? Is it normal it's not merged yet?

mkcor · 2022-11-18T15:51:10Z

Hi @mcourteaux,

I think that's because we are in between funding programs (and, hence, contracts), so we are currently short on manpower! cf. https://blog.scientific-python.org/scientific-python/2022-czi-grant/

@stefanv if you could have a look 🙏

grlee77

Thanks @mcourteaux, this looks great. I will commit the suggested small whitespace fixes then should be able to merge.

skimage/metrics/tests/test_structural_similarity.py

mcourteaux · 2022-11-18T21:16:38Z

Cool! Thanks everyone! Happy to have contributed something of this type to the scientific community. 😄

Error SSIM on data_range not specified in case of floating point data…

6b2aa00

…. Emit warnings when integers other than uint8.

Add some sanity checks on data-types and ranges of input test images.…

4311e8e

… Fix a broken test that was using structural_similarity incorrectly;

mkcor mentioned this pull request Nov 5, 2022

API updates before 0.16 #4187

Merged

9 tasks

mkcor reviewed Nov 5, 2022

View reviewed changes

Apply suggestions from @mkcor.

3424c7c

mcourteaux requested a review from mkcor November 8, 2022 13:40

mkcor approved these changes Nov 9, 2022

View reviewed changes

grlee77 approved these changes Nov 18, 2022

View reviewed changes

skimage/metrics/tests/test_structural_similarity.py Outdated Show resolved Hide resolved

skimage/metrics/tests/test_structural_similarity.py Show resolved Hide resolved

Apply suggestions from code review

55780d2

grlee77 merged commit db55f38 into scikit-image:main Nov 18, 2022

jarrodmillman added this to the 0.20 milestone Nov 18, 2022

lagru added the 🔧 type: Maintenance Refactoring and maintenance of internals label Dec 6, 2022

sfarrens mentioned this pull request Mar 15, 2023

Fix Gaussian kernel issue CEA-COSMIC/ModOpt#287

Merged

ghisvail mentioned this pull request Jul 10, 2023

MNT: Bump scikit-image to 0.21 and lxml to 4.9.3 aramis-lab/clinica#950

Merged

nickmachnik mentioned this pull request Jul 18, 2023

data_range parameter not specified - error vaquerizaslab/chess#59

Closed

Uh oh!

Conversation

mcourteaux commented Nov 4, 2022

Description

Checklist

For reviewers

Uh oh!

mcourteaux commented Nov 4, 2022

Uh oh!

mcourteaux commented Nov 4, 2022

Uh oh!

stefanv commented Nov 5, 2022

Uh oh!

mkcor commented Nov 5, 2022

Uh oh!

mkcor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mkcor Nov 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcourteaux commented Nov 8, 2022

Uh oh!

mkcor left a comment

Choose a reason for hiding this comment

Uh oh!

mcourteaux commented Nov 18, 2022

Uh oh!

mkcor commented Nov 18, 2022

Uh oh!

grlee77 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcourteaux commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants