Deprecate function `plot_matches` in favor of `plot_matched_features` by mkcor · Pull Request #7255 · scikit-image/scikit-image

mkcor · 2023-11-29T17:49:55Z

Description

Edit: The overall goal is to use image0, image1 in function signatures which have image1, image2 inputs; this PR focuses on updating function plot_matches.

Tackling an item from the to-do list which lays the ground for skimage2, i.e., "Where function signatures still use im1, im2, finally replace them with either image0, image1 or reference_image, target_image. #4187."

In some cases, e.g., where im_true is used, it might make sense to go with reference_image rather than image0. But there's clearly no reason to have image0, image1 in some places and image1, image2 in others.

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:

Deprecate function `skimage.feature.plot_matches` in favor of
`skimage.feature.plot_matched_features`.

mkcor · 2023-11-29T17:54:52Z

Before I tackle the im_true and image_true instances, I wanted to check with reviewers whether I should take 5bdeed8 further:

I searched for all the user-facing instances of image2;
I replaced the (user-facing) x1, x2 pairs with x0, x1;
should I update the source code so it's not only functional but more readable/maintainable, e.g.,
replacing

    image1 = img_as_float(image0)                                              
    image2 = img_as_float(image1)

with

    image0 = img_as_float(image0)                                              
    image1 = img_as_float(image1)

and all the rest accordingly?

stefanv · 2023-11-30T06:25:49Z

Thanks @mkcor! The inconsistencies should definitely go (0 -> 1). The x1, x2 pairs don't bother me as much, but I think it's customary in our community to count from zero.

Pretty sure there's a Monty Python joke in there somewhere, but my references fail me now.

hmaarrfk · 2023-11-30T11:23:23Z

Are these all internal functions? I feel like we are going to get into trouble with users using kwargs.

I for one, like to limit positional arguments to 2.

from wabisabi import kwarg_name_change
kwarg_name_change = partial(kwarg_name_change,
                            current_library_version=skimage.__version__,
                            library_name='skimage')


@kwarg_name_change('0.24.0', {'image1': 'image0', 'image2': 'image1', 'keypoints1': 'keypoints0', 'keypoints2': 'keypoints1'})
def foo():
...

i forget if an other decorator was done for keyword argument name changes, but I believe the one I wrote for wabisabi meets a slew of usecases

soupault · 2023-11-30T11:39:21Z

I for one, like to limit positional arguments to 2.

Very good suggestion! :) Having deprecation issues like this one in mind, and to help static code analysis in IDEs, I have personally switched to using kw-only signatures in my R&D code.

mkcor · 2023-11-30T20:08:15Z

I for one, like to limit positional arguments to 2.

👍 as well and I believe many of us are leaning towards this preference (see #7225).

To carry out a proper deprecation cycle, I guess I'll wait for an update from @lagru on #7225 (comment).

lagru · 2023-12-04T14:32:17Z

@mkcor, see the WIP at #7256.

lagru · 2023-12-04T14:34:53Z

+1 of paying close attention to how many parameters we want to make reachable via position. Not a fan of hard-coding a number. I'd rather recommend to use positional args only for parameters whose contribution to the function is obvious. I'm optimistic about our instincts on that. :)

skimage/feature/util.py

lagru · 2024-01-18T23:46:12Z

skimage/util/compare.py

 from itertools import product


-def compare_images(image1, image2, method='diff', *, n_tiles=(8, 8)):


In this case we could get away with making the arguments position-only, but I'm hesitant to really use that feature of Python. I feel like it's a lesser known aspect of Python and might confuse unnecessarily.

But I think, if we use a wrapper here that warns for a deprecation period, we can get away with eventually settling on image0, image1, cause regardless of how you used the old API and how long a user slept on the warning, the new API will always raise an error:

compare_images(a, b) # fine for old and new compare_images(image1=a, image2=b) # will raise compare_images(a, image2=b) # will raise

Thinking about this, that might go for the plot_matches above as well, but renaming might still be easier.

In this case we could get away with making the arguments position-only, but I'm hesitant to really use that feature of Python. I feel like it's a lesser known aspect of Python and might confuse unnecessarily.

Right; I had to look it up to learn that all arguments before / in the function signature means they are positional-only. I don't think we have it anywhere in the codebase (as of now).

This reverts commit 5bdeed8.

mkcor · 2024-02-16T11:02:53Z

46eca3b needs to pass, ensuring that I didn't miss any name change when copying-pasting plot_matches into plot_matched_features (7f02925).

471d79d doesn't change any functionality, it's just a formal update so the variable names read naturally now that we've moved from (1, 2) to (0, 1) naming.

mkcor · 2024-02-16T12:48:15Z

It looks like Python 3.12 treats deprecation warnings as errors: I have kept the test for (now deprecated function) plot_matches). Should I:

remove this test (now useless, since we have the same test for the new function) or
decorate this test in a specific way? @lagru do you know of a good practice for this?

mkcor · 2024-02-16T14:37:29Z

"Keyword-only arguments are not required to have a default value." PEP 3102 😌

lagru

Should I:
remove this test (now useless, since we have the same test for the new function) or
decorate this test in a specific way? @lagru do you know of a good practice for this?

I think the usual practice has been to repurpose the existing tests to the new API. So with the current state, I'd just remove the old test triggering the deprecation warning.

You could use @pytest.mark.filterwarnings as well, but why bother.

skimage/feature/tests/test_util.py

lagru · 2024-02-19T17:22:50Z

skimage/feature/tests/test_util.py

+        plot_matched_features(
+            img0,
+            img1,
+            ax=ax,
+            keypoints0=keypoints0,
+            keypoints1=keypoints1,
+            matches=matches,
+            keypoints_color='r',
+        )
+        plot_matched_features(
+            img0,
+            img1,
+            ax=ax,
+            keypoints0=keypoints0,
+            keypoints1=keypoints1,
+            matches=matches,
+            matches_color='r',
+        )


I think there's some repetition here? In my book it would be okay to shorten this even further and use pytest.mark.parametrize for the optional parameters.

I think there's some repetition here?

The difference is that one call tests for keypoints_color='r' and the other matches_color='r'.

In my book it would be okay to shorten this even further and use pytest.mark.parametrize for the optional parameters.

Hmm, that would only increase the number of tests, since pytest.mark.parametrize lets you test different values for a given parameter. Here, only one value (per optional parameter) is being tested...

Huh, I should check my eyes (or maybe take a nap). 😅 Any reason not to increase the number of tests? They don't seem slow on my machine.

Oh, I'm not opposed to it, I just didn't think it was necessary and I thought (I guess I misunderstood) you were saying there were too many already!

mkcor · 2024-02-19T21:26:40Z

I think the usual practice has been to repurpose the existing tests to the new API. So with the current state, I'd just remove the old test triggering the deprecation warning.

Thanks for your reply, @lagru. In the end, I thought that removing the 'old' test before v0.25 would artificially lower our test coverage... So I thought we could keep it until then.

lagru

Looks good! Let's leave it open for a bit to give other devs a chance to chime in about the API change.

lagru · 2024-02-22T12:49:28Z

skimage/feature/tests/test_util.py

+        matches=matches,
+        alignment='vertical',
+    )
+    plt.close()


Curious why you added these as there isn't a call to show() anywhere?

Because of this: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (`matplotlib.pyplot.figure`) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam `figure.max_open_warning`). Consider using `matplotlib.pyplot.close()`.
The number of figures opened increased after I started using pytest.mark.parametrize 😉

Ah, that seems like a very sensible change then. :D

lagru · 2024-02-22T12:52:17Z

Btw I relabeled this as an API change as it involves a deprecation. We've used "type: Maintenance" mostly to categorize internal stuff that's of no interest to users.

lagru · 2024-03-10T14:32:31Z

Merging. Thanks @mkcor! One more item of the list. ✔️

Replace image1, image2 with image0, image1

5bdeed8

mkcor added the 🔧 type: Maintenance Refactoring and maintenance of internals label Nov 29, 2023

mkcor marked this pull request as draft December 2, 2023 19:16

lagru mentioned this pull request Dec 4, 2023

Add new deprecate_parameter helper #7256

Merged

mkcor mentioned this pull request Jan 10, 2024

Handle floating point warning for empty images in skimage.registration.phase_cross_correlation #7287

Merged

lagru added the 🥾 Path to skimage2 A step towards the new "API 2.0" label Jan 18, 2024

lagru reviewed Jan 18, 2024

View reviewed changes

skimage/feature/util.py Outdated Show resolved Hide resolved

lagru reviewed Jan 18, 2024

View reviewed changes

mkcor added 4 commits February 16, 2024 11:15

Merge branch 'main' into standardize-signature

92a63d8

Revert "Replace image1, image2 with image0, image1"

b50e6a9

This reverts commit 5bdeed8.

Deprecate function plot_matches in favor of plot_matched_features

7f02925

Duplicate test for plot_matched_features

46eca3b

mkcor changed the title ~~Use image0, image1 in function signatures~~ Deprecate function plot_matches in favor of plot_matched_features Feb 16, 2024

Update variable names with new convention

471d79d

Add directives

cafcdb0

mkcor marked this pull request as ready for review February 16, 2024 12:48

mkcor added 3 commits February 16, 2024 13:58

Proofread docstring

1173cda

Limit number of positional arguments to two

b938da4

Update test accordinly

e8e4cdf

Update parameter order in docstring

373f594

Merge branch 'main' into standardize-signature

14d9b3f

mkcor mentioned this pull request Feb 16, 2024

Standardize function signature of compare_images. #7322

Merged

lagru reviewed Feb 19, 2024

View reviewed changes

mkcor added 4 commits February 19, 2024 19:27

Use pytest.mark.parametrize

d98758e

Use pytest.mark.parametrize in old test

70b4c40

Capture deprecation warnings

95221af

Close figures created through the pyplot interface

9c6aee0

lagru approved these changes Feb 22, 2024

View reviewed changes

lagru added 📜 type: API Involves API change(s) and removed 🔧 type: Maintenance Refactoring and maintenance of internals labels Feb 22, 2024

lagru merged commit 78c32ec into scikit-image:main Mar 10, 2024

stefanv added this to the 0.23 milestone Mar 10, 2024

mkcor deleted the standardize-signature branch March 11, 2024 08:54

lagru mentioned this pull request Apr 9, 2024

0.23 release #7374

Closed

lagru mentioned this pull request Aug 5, 2024

Complete deprecation of plot_matches #7487

Merged

		from itertools import product


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

Uh oh!

Conversation

mkcor commented Nov 29, 2023 • edited by lagru Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Release note

Uh oh!

mkcor commented Nov 29, 2023

Uh oh!

stefanv commented Nov 30, 2023

Uh oh!

hmaarrfk commented Nov 30, 2023

Uh oh!

soupault commented Nov 30, 2023

Uh oh!

mkcor commented Nov 30, 2023

Uh oh!

lagru commented Dec 4, 2023

Uh oh!

lagru commented Dec 4, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkcor commented Feb 16, 2024

Uh oh!

mkcor commented Feb 16, 2024

Uh oh!

mkcor commented Feb 16, 2024

Uh oh!

lagru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkcor commented Feb 19, 2024

Uh oh!

lagru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lagru commented Feb 22, 2024

Uh oh!

lagru commented Mar 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mkcor commented Nov 29, 2023 •

edited by lagru

Loading