-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
modified foerstner to return response image in line with other corner… #4159
modified foerstner to return response image in line with other corner… #4159
Conversation
Hello @zagerpatrick! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
@zagerpatrick thank you very much for helping with scikit-image! Unfortunately, we can't simply change the API here, since many users might be affected. Instead, we need to go through a deprecation cycle, as explained in our contributing document here: https://scikit-image.org/docs/dev/contribute.html#deprecation-cycle Please let us know if you would like more guidance in how to proceed. |
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.
Thank you for your work @zagerpatrick! I left a small consideration here, too.
@@ -586,7 +586,7 @@ def corner_shi_tomasi(image, sigma=1): | |||
return response | |||
|
|||
|
|||
def corner_foerstner(image, sigma=1): | |||
def corner_foerstner(image, accuracy_thresh, roundness_thresh, sigma=1): |
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 would put some default values for these two variables. How's the example values (0.5
, 0.3
) sound?
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.
Also, it would make more sense globally to have image, sigma, accuracy_thresh, roundness_thresh
, since it'd maintain the model of the other functions. Then, we wouldn't need to deprecate the API. Is that right @jni?
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.
is that right @jni?
The arguments should indeed come after and be made keyword-only by using sigma=1, *, accuracy_thresh=...
, but the API still needs to go through a deprecation cycle somehow, because the return value is being altered. I don't know what the best way to do this is...
corner_foerstner was modified to return the Foerstner corner measure response image instead of the Foerstner corner measures which are currently returned. Fixes #4000
Description
Checklist
./doc/examples
(new features only)./benchmarks
, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.@meeseeksdev backport to v0.14.x