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

rename selem to footprint everywhere #5445

Merged
merged 17 commits into from
Jun 29, 2021
Merged

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Jun 26, 2021

Description

In the discussion in #4154 it was decided that footprint should be less ambiguous than selem for use in all rank filters and morphology functions.

This PR makes that change

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 grlee77 added 🔧 type: Maintenance Refactoring and maintenance of internals 📜 type: API Involves API change(s) labels Jun 26, 2021
@pep8speaks
Copy link

pep8speaks commented Jun 26, 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-06-28 13:56:00 UTC

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

Awesome. 🙏 I've left one minor comment about the future of the API, but happy to merge this as-is and leave that discussion for later.

Comment on lines +24 to +25
footprint = _resolve_neighborhood(None, connectivity, image.ndim)
result = ndimage.label(image, structure=footprint)
Copy link
Member

Choose a reason for hiding this comment

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

Are you consciously leaving structure -> footprint to a later PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but all instances of structure as a kwarg seem to be when calling scipy.ndimage functions. I doubt there will be much enthusiasm for renaming those upstream (at least there was resistance to renaming the first argument from input to image in the past).

Copy link
Member

Choose a reason for hiding this comment

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

Oh my apologies, I thought this was measure.label. 🤦

"`skimage.morphology.selem` should be imported directly from "
"`skimage.morphology` instead.",
FutureWarning, stacklevel=2
)
Copy link
Member

Choose a reason for hiding this comment

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

Lovely.

I wonder though whether we should not instead encourage directly using from skimage.morphology import footprints; footprint = footprints.square(5). To me this addresses the point in #5439 about grouping functions according to "purpose and equivalence classes". morphology.square() is also less descriptive than footprints.square.

Perhaps something to add to a dedicated issue for discussion (tracked from #5439) rather than hold up this PR.

Copy link
Member

Choose a reason for hiding this comment

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

(But, a quick fix here would be to remain agnostic in the prescription: instead of "should be imported from skimage.morphology instead", we could write "skimage.morphology.selem has been moved to skimage.morphology.footprints.") (And yes I have a mild preference for "footprints" over "footprint", because the latter is easily confused with a variable/kwarg name.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure if this selem/footprint module within filters, morphology, etc. was really intended to be public or not. Specifically, these functions are grouped in with all other morphology functions in the docs.

I am fine with renaming to footprints.py and importing from there, but wonder if we can perhaps follow up later to make these logical groupings (binary vs. gray vs. footprints) clearer in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, following things up later is totally fine, but that's why I'm saying, let's not be prescriptive in the deprecation message, because we might change our mind during those discussions.

from skimage.morphology.grey import erosion


@pytest.mark.parametrize(
Copy link
Member

Choose a reason for hiding this comment

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

Gotta love this function 😂

@jni
Copy link
Member

jni commented Jun 28, 2021

Note: if there are no further updates I'll merge this by the end of the week.

@grlee77
Copy link
Contributor Author

grlee77 commented Jun 28, 2021

Thanks for reviewing @jni. I made the suggested rename and warning changes. As far as I could tell, the uses of structure arise of scipy.ndimage functions, not ours.

Two other things were also updated since you reviewed:
1.) renamed test_selem.py -> test_footprints.py
2.) There were still tons of mentions of "structuring element" in the docstrings! These now refer to footprint

@jni
Copy link
Member

jni commented Jun 29, 2021

Great. Totally happy with the changes now. I'll merge this in <24h if no one has stepped in to review+merge before then.

Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

Thank you @grlee77, merging 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 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.

None yet

4 participants