Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Standardize function signature of compare_images. #7322
Changes from 22 commits
216a575
4f1e326
f5ba6ee
44fdf66
32d09da
96ea4d2
32eadf6
0e8bd61
9784f80
535ed04
35c0db6
3364ae1
234438a
7d5ae50
2c51f12
4fefbc5
65177be
3237cfc
4b0c34b
e0d5929
d08d3b2
878d15c
db1b774
c723c5d
79acf77
9b9f757
1f93250
7227424
0756a56
14e27e4
0f18d56
770499a
17cfe64
c44e916
9a10712
e406346
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The wrapper shouldn't recognize the
arg
formethod
by value but by position. The function will/should handle if a wrong value is passed asmethod
.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.
Yes, the function does handle it:
scikit-image/skimage/util/compare.py
Lines 59 to 62 in e453a53
Basically I wanted to cover both cases, where a user might call:
and another one might call:
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 catchingThere 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.
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 useimage2=DEPRECATED
and we can simply use the position reliably? Right now it seems even harder to think of all edge cases. E.g. considerThis will neither warn nor error but will simply return as if the user requested
compare_images(a, b)
(defaultmethod=diff
). It should definitely error.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.
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.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.
Right; there's definitely no reason to 'handle' it within the function itself and perhaps not even through the decorator...
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.
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.
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.
In the end, I was lazy and went for 9b9f757 (not trying to figure out what the third positional argument was meant to be...).
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'm sorry but unfortunately I think looking at
deprecate_parameter
has lead you on a wrong path and over-complicated things. And also breakswhich 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
intokwargs
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 turnsmethod
into a keyword-only parameter.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.
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.
No worries! I did this quickly and thought I would iterate afterwards.
Pardon me? I just tried locally and it works; it warns, indeed, and it returns
just like current main.
method="diff"
returnsinstead. I just noticed that
method="checkerboard"
errors becausea
andb
are 1-dimensional, which doesn't work withscikit-image/skimage/util/compare.py
Line 48 in e453a53
So we should probably improve the function by checking, in the
method="checkerboard"
case, that ndim == 2 😬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.
Done db1b774. If you prefer, I can cherry-pick this commit and submit it as a separate PR.
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.
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.
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 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.
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.
Thanks, @lagru.
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.
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... 🙄
I say we remove them in skimage2 😁
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.
Might at least use
match=".*image[01]
here and elsewhere.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'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 😉