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

removed tophat and bottomhat from filters.rank #3614

Merged
merged 2 commits into from
Jan 29, 2021

Conversation

ThomasWalter
Copy link
Contributor

Description

PR in response to discussion #3599 : removed bottomhat and tophat from filters.rank. These filters were named incorrectly. They are effectively calculating internal and external morphological gradients. After discussion with @sciunto and @emmanuelle we came to the conclusion that probably, it does not make so much sense to keep these functions.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

Closes issue #3599

For reviewers

(Don't remove the checklist below.)

  • 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.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@sciunto sciunto added 🔧 type: Maintenance Refactoring and maintenance of internals 🧐 Needs review labels Dec 19, 2018
@sciunto sciunto added this to the 0.15 milestone Dec 19, 2018
@emmanuelle
Copy link
Member

If this is approved, doc/release/release_dev.rst will need to be updated as well. I'm not sure we need to use the deprecation process here since I consider these functions to be quite hidden in the library and my gut feeling is that nobody is using them. But of course, not deprecating/warning users is quite brutal, so please comment @scikit-image/core

@jni
Copy link
Member

jni commented Dec 20, 2018

Oh yeah, I don't think we can just remove remove them. Since they are incorrect I don't think we should use deprecation, but rather, try to use import hooks or at least a redefinition of them so that a NotImplementedError or similar can be raised, mentioning that they were removed in 0.14.2/0.15 because they were incorrect, pointing users to how to reimplement the "morphological gradient" (in case they were getting good results with that behaviour), and to use ndimage for tophat/bottomhat. And also maybe point to the relevant discussion in #3599. Yes it will be a long message but a long message is warranted when you pull the rug from under people!

@soupault
Copy link
Member

soupault commented Jan 19, 2019

I consider these functions to be quite hidden in the library

They are a part of public API, aren't they? (http://scikit-image.org/docs/dev/api/skimage.filters.rank.html#bottomhat, http://scikit-image.org/docs/dev/api/skimage.filters.rank.html#tophat)

redefinition of them so that a NotImplementedError or similar can be raised, mentioning that they were removed in 0.14.2/0.15 because they were incorrect, pointing users to how to reimplement the "morphological gradient"

👍 on this approach: raise NotImplementedError in 0.15, remove in 0.16. We can include a link to the current implementations (i.e. to GitHub history) into the error messages.

@JDWarner
Copy link
Contributor

Also 👍 on breaking immediately but directing any users to the new location with a brief explanation.

@sciunto sciunto modified the milestones: 0.15, 0.16 Mar 9, 2019
@sciunto
Copy link
Member

sciunto commented Mar 9, 2019

Postponed to 0.16. @ThomasWalter would mind adding an exception and a task in the todo list please?

@sciunto sciunto modified the milestones: 0.16, 0.17 Jan 29, 2020
@sciunto sciunto added the 🔽 Deprecation Involves deprecation label Apr 8, 2020
@grlee77 grlee77 added this to In progress in skimage2 API Jan 27, 2021
@grlee77
Copy link
Contributor

grlee77 commented Jan 27, 2021

I rebased this to resolve merge conflicts from a recent 3d rank filters PR. Now that 0.18 was released, I think this should be ready to merge if all is green on CI (docstrings since the 0.17 release have indicated these would be removed in 0.19)

@grlee77 grlee77 modified the milestones: 0.17, 0.19 Jan 27, 2021
@stefanv stefanv merged commit 70d8577 into scikit-image:master Jan 29, 2021
@stefanv
Copy link
Member

stefanv commented Jan 29, 2021

Thanks for your input, everyone!

@stefanv
Copy link
Member

stefanv commented Jan 29, 2021

And, of course, @ThomasWalter who stuck with this for two years ;)

@grlee77 grlee77 moved this from In progress to Done in skimage2 API Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔽 Deprecation Involves deprecation 🧐 Needs review 🔧 type: Maintenance Refactoring and maintenance of internals
Projects
Development

Successfully merging this pull request may close these issues.

None yet

9 participants