Standardize function signature of compare_images.

[mkcor]

Description

This PR is a follow-up to #7255.

Checklist

• A descriptive but concise pull request title
• Docstrings for all functions
• Unit tests
• A gallery example in ./doc/examples for new features
• Contribution guide is followed

Release note

Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically:

In `skimage.util.compare_images`, deprecate the parameter `image2`.
Instead use `image0`, `image1` to pass the compared images. Furthermore,
all other parameters will be turned into keyword-only parameters once
the deprecation is complete.

[mkcor]

I'm a little embarrassed because, with this change, existing code written as compare_images(image1=a, image2=b) will raise an error (due to image1 being also the new name of image2). To avoid breaking existing valid code (although I suspect that the vast majority of existing code would write compare_images(a, b)), I can handle the deprecation in a custom/hard-coded way, without using the deprecate_parameter decorator...

[lagru]

Thanks for tackling this @mkcor. Especially the somewhat complex documentation of the deprecation. 😅

[lagru]

Might at least use match=".*image[01] here and elsewhere.

[mkcor]

I'm not sure... I removed the case where input images are passed as kwargs following the new API (e0d5929), so we're not quite cycling over all image0-image1 combinations when expecting a warning. Or maybe my command of regular expressions is not up to the mark 😉

[mkcor]

@lagru I've drawn from your class deprecate_parameter but, as you will see, it's a bit ugly, hard-coded, etc. 🙈 I rushed to submit this, in case it could make it into v0.23. Cc @stefanv

[lagru]

I'm sorry but unfortunately I think looking at deprecate_parameter has lead you on a wrong path and over-complicated things. And also breaks

a = np.ones(10)
b = np.zeros(10)
ski.util.compare_images(a, b, "blend")
# warns but still uses method="diff"

which used to work before and should continue working during the deprecation. I think we can't use an argument with =DEPRECATED here.

Except for that I think you got quite close, especially with the idea to first turn all args into kwargs to more easily handle them. What do you think about the draft and decorator below? It should work for all cases unless I've forgotten a a way to abuse the signature. It also turns method into a keyword-only parameter.

def _rename_image_params(func):

    @functools.wraps(func)
    def wrapper(*args, **kwargs):

        # Turn all into keyword parameters
        for i, (value, param) in enumerate(
            zip(args, ["image0", "image1", "method", "n_tiles"])
        ):
            if i >= 2:
                warnings.warn("method will become keyword-only")
            if param in kwargs:
                raise ValueError(
                    f"{param} given as positional and keyword argument"
                )
            else:
                kwargs[param] = value
        args = tuple()

        # Account for `image2` if given
        if "image2" in kwargs:
            warnings.warn("used deprecated image2")

            # Safely move `image2` to `image1` if that's empty
            if "image1" in kwargs:
                # Safely move `image1` to `image0`
                if "image0" in kwargs:
                    raise ValueError(
                        "three images given, `image0`, `image1` and `image2`"
                    )
                kwargs["image0"] = kwargs.pop("image1")
            kwargs["image1"] = kwargs.pop("image2")

        return func(*args, **kwargs)

    return wrapper

Of course it doesn't have the nice convenience stuff of modifying the docstring and warnings and error messages are drafts and need to be polished. But I think this deprecation is so special that we should just edit the docstring by hand.

[mkcor]

I'm sorry but unfortunately I think looking at deprecate_parameter has lead you on a wrong path and over-complicated things.

No worries! I did this quickly and thought I would iterate afterwards.

And also breaks

a = np.ones(10)
b = np.zeros(10)
ski.util.compare_images(a, b, "blend")
# warns but still uses method="diff"

which used to work before and should continue working during the deprecation.

Pardon me? I just tried locally and it works; it warns, indeed, and it returns

array([0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5])

just like current main. method="diff" returns

array([1., 1., 1., 1., 1., 1., 1., 1., 1., 1.])

instead. I just noticed that method="checkerboard" errors because a and b are 1-dimensional, which doesn't work with

scikit-image/skimage/util/compare.py

Line 48 in e453a53

shapex, shapey = img1.shape

So we should probably improve the function by checking, in the method="checkerboard" case, that ndim == 2 😬

[mkcor]

So we should probably improve the function by checking, in the method="checkerboard" case, that ndim == 2 😬

Done db1b774. If you prefer, I can cherry-pick this commit and submit it as a separate PR.

[lagru]

Ah, you are right, it does handle this. I chose my example poorly and got confused by the image2=DEPRECATED as the third positional arg.

... which brings me to the point that we should probably remove it and make it so that the function shows the expected new signature as un-ambiguously as possible.

[lagru]

The wrapper shouldn't recognize the arg for method by value but by position. The function will/should handle if a wrong value is passed as method.

[mkcor]

Yes, the function does handle it:

scikit-image/skimage/util/compare.py

Lines 59 to 62 in e453a53

	raise ValueError(
	'Wrong value for `method`. '
	'Must be either "diff", "blend" or "checkerboard".'
	)

Basically I wanted to cover both cases, where a user might call:

compare_images(a, b, 'diff') 

and another one might call:

compare_images(a, b, c, 'diff') 

which should definitely error -- but this should also be taken care of by the function, right? I realize that my RuntimeError("Use image0, image1 to pass the two input images.") isn't catching it anyway, it's only catching

compare_images(image0=a, image1=b, image2=c, ...) 

[lagru]

That's why I think we shouldn't use the value of the 3rd and 4th argument to determine which one is method... just don't use image2=DEPRECATED and we can simply use the position reliably? Right now it seems even harder to think of all edge cases. E.g. consider

import skimage as ski
a = np.ones((10, 10)) * 3
b = np.zeros((10, 10))
ski.util.compare_images(a, b, "unknown")

This will neither warn nor error but will simply return as if the user requested compare_images(a, b) (default method=diff). It should definitely error.

[lagru]

Replying to your question: compare_images(a, b, c, 'diff') should definitely error but it's not an expected use case of the old or new API so users shouldn't really encounter this case unless they try to mess around.

[mkcor]

Replying to your question: compare_images(a, b, c, 'diff') should definitely error but it's not an expected use case of the old or new API so users shouldn't really encounter this case unless they try to mess around.

Right; there's definitely no reason to 'handle' it within the function itself and perhaps not even through the decorator...

[mkcor]

That's why I think we shouldn't use the value of the 3rd and 4th argument to determine which one is method... just don't use image2=DEPRECATED and we can simply use the position reliably? Right now it seems even harder to think of all edge cases. E.g. consider

import skimage as ski
a = np.ones((10, 10)) * 3
b = np.zeros((10, 10))
ski.util.compare_images(a, b, "unknown")

This will neither warn nor error but will simply return as if the user requested compare_images(a, b) (default method=diff). It should definitely error.

Ouch, yes. Maybe we should also check the args' types then? Let me try something based on your suggestion and ensure that yet this other edge case is covered.

[mkcor]

In the end, I was lazy and went for 9b9f757 (not trying to figure out what the third positional argument was meant to be...).

[lagru]

Sorry, @mkcor I don't want to come of as devaluing your approach. I'm happy with your solution as long as it gets all edge cases. :) There are a lot of edge cases to keep in mind and the longer I think about it the more I'm undecided which signature is actually least confusing to users during the deprecation.

Is it (a)

def compare_images(image0, image1, image2=DEPRECATED, method='diff', *, n_tiles=(8, 8))

because it's easy to see that image2 is deprecated but harder to see how method is affected? It's subtle to notice that not being able to use image2 positionally also means method can no longer be used this way.

Or is it (b)

def compare_images(image0, image1, *, method='diff', n_tiles=(8, 8))

because this one makes it easy to see that method is now a keyword-only parameter.

[mkcor]

Sorry, @mkcor I don't want to come off as devaluing your approach.

All good, @lagru! I think my code wasn't very elegant, so I definitely welcome your inputs. ❤️

Now, maybe this kind of code doesn't have to be the most elegant anyway, since we will remove it two releases later?

I haven't looked closely at your alternative yet, just because potential additional edge cases came up...

I'm happy with your solution as long as it gets all edge cases. :) There are a lot of edge cases to keep in mind

Exactly, that's the point.

and the longer I think about it the more I'm undecided which signature is actually least confusing to users during the deprecation.

Is it (a)

def compare_images(image0, image1, image2=DEPRECATED, method='diff', *, n_tiles=(8, 8))

because it's easy to see that image2 is deprecated but harder to see how method is affected? It's subtle to notice that not being able to use image2 positionally also means method can no longer be used this way.

Exactly, that's the main problem with this deprecation (more so than image1, image2 changing to image0, image1).

Or is it (b)

def compare_images(image0, image1, *, method='diff', n_tiles=(8, 8))

because this one makes it easy to see that method is now a keyword-only parameter.

This was my initial move, but you said (and rightfully so) that the old API (where method may be used positionally) should still work during the deprecation cycle... What about (c)

def compare_images(image0, image1, method='diff', *, n_tiles=(8, 8))

?

[mkcor]

Not sure we're covering all edge cases, but this implementation is definitely lighter! Thanks, @lagru. ☺️

[lagru]

@mkcor thanks for addressing the discussion! I've pushed 0756a56 to correct and test the stacklevels. I'm happy with this but left a few minor concerns to make the completing the deprecation easier.

[mkcor]

Thank you for the review and for 0756a56, @lagru! I just included minor fixes in the docstring (770499a).

[mkcor]

At yesterday's community call, @lagru and I were discussing whether we should merge this (skimage2-transition--related) PR, but we refrained so as not to interfere with the 0.23 release. @jarrodmillman could we merge it now?

[mkcor]

@lagru thank you for taking care of 17cfe64! 🙏

[lagru]

@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.

[mkcor]

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 😁

[lagru]

No idea about the new error yet:

../meson.build:1:0: ERROR: Compiler /usr/bin/clang cannot compile programs.

But it seems unrelated since this PR doesn't touch any part of the infrastructure..

[jarrodmillman]

Will merge once #7407 is sorted out and merged. Thanks!!