Skip to content
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

morphology: rename files with "grey" in filename #5386

Merged
merged 7 commits into from
May 16, 2021

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented May 11, 2021

Description

closes #2604 (once #5385 is also merged)

This PR just renames a couple of the files in skimage.morphology. A file with the old name was also kept, but will raise a warning if a user imports from it. This was done for backward compatibility with existing code that may have imported directly from skimage.morphology.grey or skimage.morphology.greyreconstruct instead of from the intended skimage.morphology.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

keep functions available from old grey locations, but warn if imported from there
@grlee77 grlee77 added 🔧 type: Maintenance Refactoring and maintenance of internals 🔽 Deprecation Involves deprecation 📜 type: API Involves API change(s) labels May 11, 2021
@grlee77 grlee77 added this to the 0.19 milestone May 11, 2021
@pep8speaks
Copy link

pep8speaks commented May 11, 2021

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-13 12:36:02 UTC

@mkcor
Copy link
Member

mkcor commented May 11, 2021

Why doesn't skimage/morphology/grey.py --> skimage/morphology/gray.py appear as file renaming?

@grlee77
Copy link
Contributor Author

grlee77 commented May 11, 2021

Why doesn't skimage/morphology/grey.py --> skimage/morphology/gray.py appear as file renaming?

I think it does if you look at the first commit only. Subsequently I introduced a new file under the name grey.py that just imports the functions from the (renamed) gray.py. I think this is confusing the diff when reviewing the combined changes.

@mkcor
Copy link
Member

mkcor commented May 12, 2021

I think it does if you look at the first commit only. Subsequently I introduced a new file under the name grey.py that just imports the functions from the (renamed) gray.py. I think this is confusing the diff when reviewing the combined changes.

Ok, thanks. Yes, reviewing would be more straightforward with the 'expected' diff but I'll find a way :)

Can you please take care of the now conflicts in skimage/morphology/grey.py? Thanks 🙂

@grlee77
Copy link
Contributor Author

grlee77 commented May 12, 2021

Yes, reviewing would be more straightforward with the 'expected' diff but I'll find a way :)

The original files are unchanged aside from the file rename and string replacement of "grey" -> "gray"
The new "grey.py" etc are short, so shouldn't be too hard to review. The new parts are replacement of the names in the tests and checking that importing from the old location raises a warning.

@mkcor
Copy link
Member

mkcor commented May 12, 2021

The original files are unchanged aside from the file rename and string replacement of "grey" -> "gray"

👌 👍

The new "grey.py" etc are short, so shouldn't be too hard to review. The new parts are replacement of the names in the tests and checking that importing from the old location raises a warning.

Yes, yes, just like in #5385 (that's why I'm reviewing them in the same wake, it's cognitively efficient).

Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my suggestions are not directly related to the scope of this PR, so this is good to go!

PS: I'm disappointed to find out that we won't be 'consistent' anymore with scipy.ndimage, which uses the 'grey' spelling for their morphology functions.

skimage/morphology/gray.py Outdated Show resolved Hide resolved
skimage/morphology/gray.py Outdated Show resolved Hide resolved
skimage/morphology/gray.py Outdated Show resolved Hide resolved
skimage/morphology/gray.py Outdated Show resolved Hide resolved
skimage/morphology/gray.py Outdated Show resolved Hide resolved
skimage/morphology/gray.py Outdated Show resolved Hide resolved
skimage/morphology/gray.py Outdated Show resolved Hide resolved
skimage/morphology/tests/test_reconstruction.py Outdated Show resolved Hide resolved
skimage/morphology/gray.py Outdated Show resolved Hide resolved
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@alexdesiqueira alexdesiqueira merged commit 9659858 into scikit-image:main May 16, 2021
@alexdesiqueira
Copy link
Member

Looks gray-t to me 😄 Thank you @grlee77!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔽 Deprecation Involves deprecation 📜 type: API Involves API change(s) 🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SKIP: gray vs grey
4 participants