[lilisako]

Description

• closes metric.structural_similarity throws the wrong error for small sizes #5366

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.

[pep8speaks]

Hello @lilisako! 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 2021-07-15 14:52:02 UTC

[mkcor]

Can you also add a test for when win_size exceeds image extent? Thanks!

[lilisako]

@mkcor Thanks for the review!
I think it was already added here!

scikit-image/skimage/metrics/tests/test_structural_similarity.py

Lines 233 to 235 in e9b086d

	# win_size exceeds image extent
	with testing.raises(ValueError):
	structural_similarity(X, X, win_size=X.shape[0] + 1)

If I am misunderstanding something, please let me know!

[mkcor]

Oh, right! My intention would be to distinguish between the two cases... maybe:

    # win_size exceeds image extent                                            
    with testing.raises(ValueError) as cm:                                      
        structural_similarity(X, X, win_size=X.shape[0] + 1)
        assert 'win_size exceeds image extent' in cm.exception.msg

and

    # image is too small                                                       
    with testing.raises(ValueError) as cm:                                     
        structural_similarity(Z, Z)                                            
        assert 'Image is too small' in cm.exception.msg

?

[lilisako]

I see! Thanks for kindly letting me know. I just pushed new commits regarding these changes.

[mkcor]

Hi @lilisako,

I've thought about the update and I wouldn't actually make this if/else distinction; it's quite artificial: You could have 6x6 or 5x5 images and compute their structural similarity, it's just that you would need to specify win_size=5 or win_size=3. I think I would leave it as one error message, describing several possibilities, mentioning the multichannel case last.

Indeed,

from skimage.metrics import structural_similarity
import numpy as np

im1 = np.ones((5, 5))
im2 = np.zeros((5, 5))

structural_similarity(im1, im2, win_size=3)
structural_similarity(im1, im2, win_size=5)
structural_similarity(im1, im2)  # ValueError (it is True that "win_size exceeds image extent.")

Therefore, I would suggest:
"win_size exceeds image extent. Either ensure that your images are at least 7x7; or pass win_size explicitly in the function call, with an odd value less or equal to the smaller side of your images. If your images are multichannel (with color channels), set channel_axis to the axis number corresponding to the channels."

Best,
Marianne

[lilisako]

Hi, @mkcor !
Sorry for being late to get back to you. I changed the error message and I explicitly added the test cases for small images. Could you re-review the code when you have time?

Thanks in advance.
Risako

[mkcor]

Hi @lilisako,

Thank you for your patience! I left some suggestions when reviewing your latest version. In case you're not familiar with it, let me draw your attention to the possibility of applying suggested changes in batches: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

[lilisako]

@mkcor
Thanks for the review! I've just pushed the suggestions into one commit. I appreciate for taking time to correct these changes🙏

[alexdesiqueira]

Thank you @lilisako!

[Pomax]

small note. but: python does not want commas here. When splitting a long string over multiple lines, there should be no commas, as adding them means you're now specifying multiple arguments to the ValueError constructor, instead of a single argument.

[mkcor]

Very true, good catch! Waiting for your PR to fix this 😄

[Pomax]

See the issue this closes as for why I won't be submitting that. But if you think it's just a matter of seconds to make one, you're more than welcome to do that instead =)

[lilisako]

Thanks @Pomax for pointing out the issue. I fixed this in newly created PR here #5480 .