-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
MAINT: warns for new multichannel default in denoise_{bilateral, nl_means} #2467
Conversation
@stsievert You need to follow the procedure that we started to write here: #2422 Thx! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing, and I think we are good :)
@@ -75,7 +75,10 @@ def denoise_bilateral(image, win_size=None, sigma_color=None, sigma_spatial=1, | |||
>>> noisy = np.clip(noisy, 0, 1) | |||
>>> denoised = denoise_bilateral(noisy, sigma_color=0.05, sigma_spatial=15) | |||
""" | |||
warn('denoise_bilateral will default to multichannel=False') | |||
if multichannel is None: | |||
warn('denoise_bilateral will default to multichannel=False') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, specify the version here (0.15).
At the end, if you can squash the commit, would be awesome ;) |
You also need to update the test suite and specify the option everywhere. |
d2b4899
to
89abfff
Compare
Current coverage is 90.71% (diff: 100%)@@ master #2467 diff @@
==========================================
Files 304 304
Lines 21500 21511 +11
Methods 0 0
Messages 0 0
Branches 1851 1853 +2
==========================================
+ Hits 19502 19513 +11
Misses 1658 1658
Partials 340 340
|
89abfff
to
d2f0383
Compare
…ests uses process described in scikit-image#2422
d2f0383
to
d2e5761
Compare
Rebased and (I believe) ready to merge. |
@scikit-image/core We need a second review here. It's easy :) |
Thanks @stsievert! and @sciunto, you were right, that was easy. =D |
Fixes #2465
Description
In #2461 (comment) we realized that
denoise_wavelet
should default tomultichannel=False
. We chose this primarily becausemultichannel=False
works well for 2D images, is explicit and doesn't prioritize multichannel images (e.g., RGB images) over 2D images.From this we decided that
denoise_bilateral
should default tomultichannel=False
to provide a consistent interface with the multichannel found indenoise_{wavelet, tv_chambolle}
.denoise_nl_means
also defaults tomultichannel=True
and though not discussed this is changed as well.Checklist
References
This closes #2465.