-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix wrong error for metric.structural_similarity when image is too small #5395
Conversation
@@ -239,3 +240,6 @@ def test_invalid_input(): | |||
structural_similarity(X, X, K2=-0.1) | |||
with testing.raises(ValueError): | |||
structural_similarity(X, X, sigma=-1.0) | |||
# image is too small |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a test for when win_size exceeds image extent? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Thanks for kindly letting me know. I just pushed new commits regarding these changes.
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 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: Best, |
Hi, @mkcor ! Thanks in advance. |
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 |
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@mkcor |
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Thank you @lilisako! |
"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 than or equal to the smaller side of your ", | ||
"images. If your images are multichannel ", | ||
"(with color channels), set channel_axis to ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true, good catch! Waiting for your PR to fix this 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.