Skip to content

Deprecate old threshold_adaptive API#2490

Merged
soupault merged 11 commits intoscikit-image:masterfrom
jni:threshold-depr
Feb 7, 2017
Merged

Deprecate old threshold_adaptive API#2490
soupault merged 11 commits intoscikit-image:masterfrom
jni:threshold-depr

Conversation

@jni
Copy link
Copy Markdown
Member

@jni jni commented Feb 6, 2017

Description

This PR makes all thresholding functions consistent by returning (by default) a value threshold that can be used to threshold the original image with thresholded = image > threshold. The value may be a scalar or an array with the same shape as the input image.

See discussion in #2121 for alternate APIs. However, this API is consistent with the current API for almost all our thresholding functions, so I think it should be strongly preferred, and it should certainly be preferred for the purposes of the 0.13 release.

Checklist

References

Closes #2121

@jni jni added this to the 0.13 milestone Feb 6, 2017
@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 6, 2017

Codecov Report

Merging #2490 into master will increase coverage by <.01%.

@@            Coverage Diff             @@
##           master    #2490      +/-   ##
==========================================
+ Coverage   90.69%   90.69%   +<.01%     
==========================================
  Files         304      304              
  Lines       21527    21542      +15     
  Branches     1859     1860       +1     
==========================================
+ Hits        19523    19538      +15     
  Misses       1661     1661              
  Partials      343      343
Impacted Files Coverage Δ
skimage/filters/init.py 100% <100%> (ø)
skimage/filters/thresholding.py 94.22% <100%> (+0.1%)
...kimage/measure/tests/test_structural_similarity.py 100% <100%> (ø)
skimage/filters/tests/test_thresholding.py 100% <100%> (ø)
skimage/measure/_structural_similarity.py 98.09% <100%> (ø)
skimage/_shared/utils.py 86.84% <100%> (+0.54%)
skimage/util/tests/test_apply_parallel.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d8cbe6...973e8ac. Read the comment docs.

@soupault soupault added ⏩ type: Enhancement Improve existing features 🔧 type: Maintenance Refactoring and maintenance of internals labels Feb 6, 2017
Copy link
Copy Markdown
Member

@sciunto sciunto left a comment

Choose a reason for hiding this comment

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

I left a minor remark. Great PR.

def threshold_adaptive(image, block_size, method='gaussian', offset=0,
mode='reflect', param=None):
warn('The return value of `threshold_local` is not the same as that '
'of `threshold_adaptive`.')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps we can be more precise here by saying that threshold_adaptive returns a thresholded image while threshold_local return the threshold values.

@jni
Copy link
Copy Markdown
Member Author

jni commented Feb 6, 2017

@sciunto good suggestion, done! I hope... =)

Copy link
Copy Markdown
Member

@sciunto sciunto left a comment

Choose a reason for hiding this comment

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

Perfect! Approved :)

def threshold_adaptive(image, block_size, method='gaussian', offset=0,
mode='reflect', param=None):
warn('The return value of `threshold_local` is a threshold image, while '
'`threshold_adaptive` returned the *thresholded* image.')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please, add also a warning saying that the function is deprecated and will be removed in 0.15.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The decorator miust do it... but not the version, you're right

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm... Perhaps, we could add a version argument to the deprecated class 🤔 .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@jni
Copy link
Copy Markdown
Member Author

jni commented Feb 6, 2017

@soupault @sciunto indeed when I was writing this I was wondering why the decorator didn't have a few more features. Hopefully this is done now! =)

@jni
Copy link
Copy Markdown
Member Author

jni commented Feb 7, 2017

This should be ready now!

@soupault
Copy link
Copy Markdown
Member

soupault commented Feb 7, 2017

Such a good PR! Thanks @jni !

@soupault soupault merged commit 30a19d0 into scikit-image:master Feb 7, 2017
@jni jni deleted the threshold-depr branch February 7, 2017 12:21
@adius adius mentioned this pull request Apr 19, 2017
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⏩ type: Enhancement Improve existing features 🔧 type: Maintenance Refactoring and maintenance of internals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants