-
-
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
Slight shape_index
docstring modification to specify 2D array
#5509
Conversation
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 @Yash-10
I made a suggestion, but it would be good for other maintainers to confirm if this is indeed the style we settled on (I did not look up the issue just now)
skimage/feature/corner.py
Outdated
@@ -455,7 +455,7 @@ def shape_index(image, sigma=1, mode='constant', cval=0): | |||
|
|||
Parameters | |||
---------- | |||
image : ndarray | |||
image : 2D array |
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.
image : 2D array | |
image : (M, N) ndarray |
I think this should probably be (M, N) ndarray
, but haven't tracked down the issue where we discussed the style to use previously.
None of the functions in this corner.py
file use that style, but many others in the library do (See for example haar.py
, match.py
, orb.py
in this same folder)
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 for the suggestion!
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 can not either remember if we already specified the style to use, but I think that you are right @grlee77: the style you are suggesting is already used in many places in the repo.
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. Apparently, we also have:
scikit-image/skimage/morphology/gray.py
Line 25 in 3ad3641
footprint : 2D array, shape (M, N) |
in some places. Let's stick with
(M, N) ndarray
.
f8941b4
to
84673a8
Compare
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 @Yash-10 for your contribution 😉
Description
I was recently exploring the
shape_index
function and found some discussion here: https://mail.python.org/archives/list/scikit-image@python.org/thread/Q5FNUVXJBCB7YDAGHKTO7EUQORR7WMGF/ where, I think, @emmanuelle had suggested changing the docstring to explicitly state that 2D arrays are expected. From what I see, this change seems to not have been added yet, so I have tried to do it here.Thanks!
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
.