Deprecate function plot_matches in favor of plot_matched_features
#7255
Conversation
|
Before I tackle the
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? |
|
Thanks @mkcor! The inconsistencies should definitely go (0 -> 1). The Pretty sure there's a Monty Python joke in there somewhere, but my references fail me now. |
|
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 |
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. |
👍 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). |
|
+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/util/compare.py
Outdated
| @@ -4,15 +4,15 @@ | |||
| from itertools import product | |||
|
|
|||
|
|
|||
| def compare_images(image1, image2, method='diff', *, n_tiles=(8, 8)): | |||
There was a problem hiding this comment.
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 raiseThinking about this, that might go for the plot_matches above as well, but renaming might still be easier.
There was a problem hiding this comment.
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).
image0, image1 in function signaturesplot_matches in favor of plot_matched_features
|
It looks like Python 3.12 treats deprecation warnings as errors: I have kept the test for (now deprecated function)
|
|
"Keyword-only arguments are not required to have a default value." PEP 3102 😌 |
There was a problem hiding this comment.
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
Outdated
| 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', | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.parametrizefor 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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
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. |
| matches=matches, | ||
| alignment='vertical', | ||
| ) | ||
| plt.close() |
There was a problem hiding this comment.
Curious why you added these as there isn't a call to show() anywhere?
There was a problem hiding this comment.
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 😉
There was a problem hiding this comment.
Ah, that seems like a very sensible change then. :D
|
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. |
Description
Edit: The overall goal is to use
image0, image1in function signatures which haveimage1, image2inputs; this PR focuses on updating functionplot_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 eitherimage0, image1orreference_image, target_image. #4187."In some cases, e.g., where
im_trueis used, it might make sense to go withreference_imagerather thanimage0. But there's clearly no reason to haveimage0, image1in some places andimage1, image2in others.Checklist
./doc/examplesfor new featuresRelease 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: