Skip to content
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

Standardize function signature of compare_images. #7322

Merged
merged 36 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
216a575
Remove obsolete instruction for documenting changes
mkcor Feb 16, 2024
4f1e326
Merge branch 'main' of github.com:scikit-image/scikit-image
mkcor Feb 16, 2024
f5ba6ee
Deprecate parameter image2 and limit to two positional arguments
mkcor Feb 16, 2024
44fdf66
Add test for replaced parameters
mkcor Feb 16, 2024
32d09da
Use pytest
mkcor Feb 16, 2024
96ea4d2
Cover all cases
mkcor Feb 16, 2024
32eadf6
Remove unnecessary default value
mkcor Feb 16, 2024
0e8bd61
Add directives for future release
mkcor Feb 16, 2024
9784f80
Ensure valid existing code does not break
mkcor Feb 19, 2024
535ed04
Handle deprecation in a custom way
mkcor Feb 19, 2024
35c0db6
Capture all deprecation warnings
mkcor Feb 19, 2024
3364ae1
Fix typo in test name
mkcor Feb 19, 2024
234438a
Update instructions for future release
mkcor Feb 19, 2024
7d5ae50
Merge branch 'main' of github.com:scikit-image/scikit-image
mkcor Feb 28, 2024
2c51f12
Keep signature compatible with old API
mkcor Mar 1, 2024
4fefbc5
Merge branch 'main' of github.com:scikit-image/scikit-image
mkcor Mar 1, 2024
65177be
Update TODO
mkcor Mar 1, 2024
3237cfc
Merge branch 'main' into image0-image1
mkcor Mar 1, 2024
4b0c34b
Merge branch 'main' into image0-image1
mkcor Mar 20, 2024
e0d5929
Remove test for new API
mkcor Mar 21, 2024
d08d3b2
Remove warning test cases for new API
mkcor Mar 21, 2024
878d15c
Write custom deprecation class
mkcor Mar 21, 2024
db1b774
Ensure images are 2D for method=checkerboard
mkcor Mar 21, 2024
c723c5d
Merge branch 'main' into image0-image1
mkcor Mar 21, 2024
79acf77
Add edge case with positional args only
mkcor Mar 21, 2024
9b9f757
Account for method-related warning
mkcor Mar 21, 2024
1f93250
Refactor decorator
mkcor Mar 21, 2024
7227424
Merge branch 'main' into image0-image1
mkcor Mar 23, 2024
0756a56
Correct and test stacklevel of deprecation warnings
lagru Mar 23, 2024
14e27e4
Update TODO.txt
mkcor Mar 25, 2024
0f18d56
Apply suggestions from code review
mkcor Mar 25, 2024
770499a
Fix typos
mkcor Mar 25, 2024
17cfe64
Update deprecation messages to 0.24
lagru Apr 23, 2024
c44e916
Merge branch 'main' into pr/7322_image0-image1
lagru Apr 23, 2024
9a10712
Use correct indent for versionchanged directive
lagru Apr 24, 2024
e406346
Merge branch 'main' into pr/7322_image0-image1
lagru May 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions TODO.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ Version 0.25
and remove `test_gaussian.py::test_deprecated_gaussian_output`.
Make arguments after the deprecated `output` parameter, keyword only:
`gaussian(image, sigma, *, ...)`.
* In `skimage/util/compare.py`, remove deprecated parameter `image2` as
well as `test_compare_images_replaced_param` in `skimage/util/tests/test_compare.py`
(and all `pytest.warns(FutureWarning)` context managers there).

Version 0.26
------------
Expand Down
83 changes: 76 additions & 7 deletions skimage/util/compare.py
Original file line number Diff line number Diff line change
@@ -1,23 +1,85 @@
import functools
import warnings
from itertools import product

import numpy as np

from .dtype import img_as_float
from itertools import product


def compare_images(image1, image2, method='diff', *, n_tiles=(8, 8)):
def _rename_image_params(func):
wm_images = (
"Since version 0.24, the two input images are named `image0` and "
"`image1` (instead of `image1` and `image2`, respectively). Please use "
"`image0, image1` to avoid this warning for now, and avoid an error "
"from version 0.26 onwards."
)

wm_method = (
"Starting in version 0.24, all arguments following `image0, image1` "
"(including `method`) will be keyword-only. Please pass `method=` "
"in the function call to avoid this warning for now, and avoid an error "
"from version 0.26 onwards."
)

@functools.wraps(func)
def wrapper(*args, **kwargs):
# Turn all args into kwargs
for i, (value, param) in enumerate(
zip(args, ["image0", "image1", "method", "n_tiles"])
):
if i >= 2:
warnings.warn(wm_method, category=FutureWarning, stacklevel=2)
if param in kwargs:
raise ValueError(
f"{param} passed both as positional and keyword argument."
)
else:
kwargs[param] = value
args = tuple()

# Account for `image2` if given
if "image2" in kwargs.keys():
warnings.warn(wm_images, category=FutureWarning, stacklevel=2)

# Safely move `image2` to `image1` if that's empty
if "image1" in kwargs.keys():
# Safely move `image1` to `image0`
if "image0" in kwargs.keys():
raise ValueError(
"Three input images given; please use only `image0` "
"and `image1`."
)
kwargs["image0"] = kwargs.pop("image1")
kwargs["image1"] = kwargs.pop("image2")

return func(*args, **kwargs)

return wrapper


@_rename_image_params
def compare_images(image0, image1, *, method='diff', n_tiles=(8, 8)):
"""
Return an image showing the differences between two images.

.. versionadded:: 0.16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkcor I corrected a minor indent issue in 9a10712. Also, I'm thinking about how we can reduce the visual noise these kind of directives create in our docs. E.g. this directive I'm commenting on doesn't seem all that useful. We should probably reserve using them for breaking changes when they are absolutely necessary to avoid user confusion. And do we plan on removing them ever? Curious what you think. :)

Let's merge once the CI is green.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @lagru.

E.g. this directive I'm commenting on doesn't seem all that useful.

I guess it was useful back when the latest release was 0.16, 0.17, ... but it's losing this character of usefulness as time goes by, for sure.

We should probably reserve using them for breaking changes when they are absolutely necessary to avoid user confusion.

I guess it's fair to be informative, even if the information isn't technically 'necessary.' When is it information, when does it become noise...? I can hear that we want to strike the right balance.

I thought that, since these directives existed, they were probably very useful somewhere somewhat... 🙄

And do we plan on removing them ever?

I say we remove them in skimage2 😁


Parameters
----------
image1, image2 : ndarray, shape (M, N)
image0, image1 : ndarray, shape (M, N)
Images to process, must be of the same shape.

.. versionchanged:: 0.24
`image1` and `image2` were renamed into `image0` and `image1`
respectively.
method : string, optional
Method used for the comparison.
Valid values are {'diff', 'blend', 'checkerboard'}.
Details are provided in the note section.

.. versionchanged:: 0.24
This parameter and following ones are keyword-only.
n_tiles : tuple, optional
Used only for the `checkerboard` method. Specifies the number
of tiles (row, column) to divide the image.
Expand All @@ -32,19 +94,26 @@ def compare_images(image1, image2, method='diff', *, n_tiles=(8, 8)):
``'diff'`` computes the absolute difference between the two images.
``'blend'`` computes the mean value.
``'checkerboard'`` makes tiles of dimension `n_tiles` that display
alternatively the first and the second image.
alternatively the first and the second image. Note that images must be
2-dimensional to be compared with the checkerboard method.
"""
if image1.shape != image2.shape:

if image1.shape != image0.shape:
raise ValueError('Images must have the same shape.')

img1 = img_as_float(image1)
img2 = img_as_float(image2)
img1 = img_as_float(image0)
img2 = img_as_float(image1)

if method == 'diff':
comparison = np.abs(img2 - img1)
elif method == 'blend':
comparison = 0.5 * (img2 + img1)
elif method == 'checkerboard':
if img1.ndim != 2:
raise ValueError(
'Images must be 2-dimensional to be compared with the '
'checkerboard method.'
)
shapex, shapey = img1.shape
mask = np.full((shapex, shapey), False)
stepx = int(shapex / n_tiles[0])
Expand Down
61 changes: 50 additions & 11 deletions skimage/util/tests/test_compare.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
import numpy as np

from skimage._shared.testing import assert_array_equal
from skimage._shared import testing
import pytest

from skimage.util.compare import compare_images
from skimage._shared.testing import assert_stacklevel


def test_compate_images_ValueError_shape():
def test_compare_images_ValueError_shape():
img1 = np.zeros((10, 10), dtype=np.uint8)
img2 = np.zeros((10, 1), dtype=np.uint8)
with testing.raises(ValueError):
with pytest.raises(ValueError):
compare_images(img1, img2)


def test_compare_images_ValueError_args():
a = np.ones((10, 10)) * 3
b = np.zeros((10, 10))
with pytest.raises(ValueError):
compare_images(a, b, method="unknown")


def test_compare_images_diff():
img1 = np.zeros((10, 10), dtype=np.uint8)
img1[3:8, 3:8] = 255
Expand All @@ -21,7 +27,40 @@ def test_compare_images_diff():
expected_result = np.zeros_like(img1, dtype=np.float64)
expected_result[3:8, 0:3] = 1
result = compare_images(img1, img2, method='diff')
assert_array_equal(result, expected_result)
np.testing.assert_array_equal(result, expected_result)


def test_compare_images_replaced_param():
img1 = np.zeros((10, 10), dtype=np.uint8)
img1[3:8, 3:8] = 255
img2 = np.zeros_like(img1)
img2[3:8, 0:8] = 255
expected_result = np.zeros_like(img1, dtype=np.float64)
expected_result[3:8, 0:3] = 1

regex = ".*Please use `image0, image1`.*"
with pytest.warns(FutureWarning, match=regex) as record:
result = compare_images(image1=img1, image2=img2)
assert_stacklevel(record)
np.testing.assert_array_equal(result, expected_result)

with pytest.warns(FutureWarning, match=regex) as record:
result = compare_images(image0=img1, image2=img2)
assert_stacklevel(record)
np.testing.assert_array_equal(result, expected_result)

with pytest.warns(FutureWarning, match=regex) as record:
result = compare_images(img1, image2=img2)
assert_stacklevel(record)
np.testing.assert_array_equal(result, expected_result)

# Test making "method" keyword-only here as well
# so whole test can be removed in one go
regex = ".*Please pass `method=`.*"
with pytest.warns(FutureWarning, match=regex) as record:
result = compare_images(img1, img2, "diff")
assert_stacklevel(record)
np.testing.assert_array_equal(result, expected_result)


def test_compare_images_blend():
Expand All @@ -33,7 +72,7 @@ def test_compare_images_blend():
expected_result[3:8, 3:8] = 1
expected_result[3:8, 0:3] = 0.5
result = compare_images(img1, img2, method='blend')
assert_array_equal(result, expected_result)
np.testing.assert_array_equal(result, expected_result)


def test_compare_images_checkerboard_default():
Expand All @@ -45,9 +84,9 @@ def test_compare_images_checkerboard_default():
exp_row2 = np.array([1., 1., 0., 0., 1., 1., 0., 0., 1., 1., 0., 0., 1., 1., 0., 0.])
# fmt: on
for i in (0, 1, 4, 5, 8, 9, 12, 13):
assert_array_equal(res[i, :], exp_row1)
np.testing.assert_array_equal(res[i, :], exp_row1)
for i in (2, 3, 6, 7, 10, 11, 14, 15):
assert_array_equal(res[i, :], exp_row2)
np.testing.assert_array_equal(res[i, :], exp_row2)


def test_compare_images_checkerboard_tuple():
Expand All @@ -61,6 +100,6 @@ def test_compare_images_checkerboard_tuple():
[1.0, 1.0, 0.0, 0.0, 1.0, 1.0, 0.0, 0.0, 1.0, 1.0, 0.0, 0.0, 1.0, 1.0, 0.0, 0.0]
)
for i in (0, 1, 2, 3, 8, 9, 10, 11):
assert_array_equal(res[i, :], exp_row1)
np.testing.assert_array_equal(res[i, :], exp_row1)
for i in (4, 5, 6, 7, 12, 13, 14, 15):
assert_array_equal(res[i, :], exp_row2)
np.testing.assert_array_equal(res[i, :], exp_row2)