Skip to content

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

Merged
alexdesiqueira merged 7 commits intoscikit-image:mainfrom
grlee77:gray_mophology
May 16, 2021
Merged

morphology: rename files with "grey" in filename#5386
alexdesiqueira merged 7 commits intoscikit-image:mainfrom
grlee77:gray_mophology

Conversation

@grlee77
Copy link
Copy Markdown
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.

@grlee77
morphology: rename files with grey in filename
95a0901 
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
Copy Markdown

pep8speaks commented May 11, 2021 •
edited
Loading

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
Copy Markdown
Member

mkcor commented May 11, 2021

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

@grlee77
Copy link
Copy Markdown
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.

@grlee77 grlee77 mentioned this pull request May 11, 2021
Open
@mkcor
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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).

mkcor
mkcor approved these changes May 12, 2021
View reviewed changes
Copy link
Copy Markdown
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.

grlee77
grlee77 commented May 13, 2021
View reviewed changes
@grlee77 @mkcor
Apply suggestions from code review
b99affd 
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
Copy Markdown
Member

alexdesiqueira commented May 16, 2021

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

@mkcor mkcor mentioned this pull request May 17, 2021
Closed
@grlee77 grlee77 deleted the gray_mophology branch July 8, 2021 20:39
@crisluengo crisluengo mentioned this pull request Dec 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkcor mkcor mkcor approved these changes

Assignees

No one assigned

Labels

🔽 Deprecation Involves deprecation 📜 type: API Involves API change(s) 🔧 type: Maintenance Refactoring and maintenance of internals

Projects

None yet

Milestone

0.19

Development

Successfully merging this pull request may close these issues.

SKIP: gray vs grey

4 participants

@grlee77 @pep8speaks @mkcor @alexdesiqueira