Complete deprecations targeting release 0.20 or 1.0#6583
Complete deprecations targeting release 0.20 or 1.0#6583grlee77 merged 29 commits intoscikit-image:mainfrom
Conversation
in favor of footprint. Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
|
Thanks for working through these! |
skimage/_shared/filters.py
Outdated
| def gaussian(image, sigma=1, output=None, mode='nearest', cval=0, | ||
| multichannel=None, preserve_range=False, truncate=4.0, *, | ||
| preserve_range=False, truncate=4.0, *, | ||
| channel_axis=None): |
There was a problem hiding this comment.
I'd like to point out
scikit-image/skimage/_shared/filters.py
Lines 107 to 112 in 8ae85b2
I am pretty sure that the logic makes it impossible to indicate to the function to treat an image of shape (M, N, 3) as a grayscale image as channel_axis=None is replaced with channel_axis=-1. The warning text implies, that in the absence of an explicit multichannel, the image is treated as RGB in some cases. However, with the switch to channel_axis we can no longer replicate this behavior. channel_axis=None means "no RGB" as opposed to multichannel=None which means "not set".
Pretty sure this was broken since the deprecate_multichannel_kwarg was applied as well. A that one translates multichannel=False into channel_axis=None...
My proposal is to remove this (broken) implicit RGB assumption if (M, N, 3).
There was a problem hiding this comment.
Oops! The warning message is problematic too; it should have been updated ever since multichannel became deprecated, so the user isn't invited to set a deprecated kwarg...
There was a problem hiding this comment.
We need another None-proxy, e.g.:
class NoChannelAxis:
pass
def gaussian(..., channel_axis=NoChannelAxis):
if channel_axis is NoChannelAxis:
...
# User can do:
gaussian(..., channel_axis=None)There was a problem hiding this comment.
See a5421d5. Patching the __repr__ with a metaclass was actually a lot of fun, although it probably shouldn't be. 😅
I think the patch addresses the following cases:
- User neither passed
multichannelnorchannel_axisand relied on automatic detection of color channel for(M, N, 3). Since v0.19 we silently broke his workflow. This patch recovers the original behavior for one release and notifies the user. - User continued using
multichannel=False. Since v0.19 that was replaced withchannel_axis=None. In case the shape was(M, N, 3)we broke his code otherwise he was fine. This user is notified as well becausemultichannelis no longer available. - User read the misleading RuntimeWarning, and nevertheless started using
channel_axis=Nonefor a grayscale image. The warning continued. In case the shape was(M, N, 3)we broke his code otherwise he was fine. This patch recovers the original behavior but does not notify him.
All other cases using grayscale images shouldn't have been impacted.
There was a problem hiding this comment.
Are you asking about the status of this specific comment thread? It's been addressed in a5421d5.
Concerning the status of this PR, I think I addressed everything I found at the time. The only remaining suggestion was to add .. versionchanged:: directives everywhere. But that should not hold up this PR in my opinion and can be done here or in another PR if someone finds the time.
There was a problem hiding this comment.
Thank you for patching up this behavior @lagru, it looks great.
in favor of "out". Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
in favor of "max_num_iter". Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
in favor of "max_num_iter". Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
in favor of "num_iter". Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
in favor of "max_num_iter". Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
in favor of "label_image". Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
Though removed_version points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
Though the default removed_version of deprecate_multichannel_kwarg points at 1.0, the deprecation is finished in 0.20 which was not planned at the time of 0.19's release.
This decorator is no longer necessary since the deprecation of multichannel is completed with 0.20 (previously 1.0). Make test's independent of temporary decorations of library functions such as hog and pyramid_gaussian. Their deprecation decorator was removed.
and remove unused imports.
For filters.inverse(), the removed_version targets 0.21 and not 0.20. So move.
I think removed_version="1.2" is actually supposed to be 1.0 (or 0.20).
These deprecations are already part of 0.19. So it should be fine to remove them in 0.20.
7462c13 to
b53b46a
Compare
Missed this earlier.
|
@scikit-image/core this is ready for review. Please have a look. Reviewing commit by commit might be useful as I tried to keep related changes together... The fix for the failing test is discussed in #6583 (comment). |
|
|
||
|
|
||
| @deprecate_kwarg(kwarg_mapping={'selem': 'footprint'}, removed_version="1.0", | ||
| deprecated_version="0.19") |
There was a problem hiding this comment.
Before the 'Returns' section, shouldn't there be something like:
.. versionchanged:: 0.19
Parameter ``selem`` has been replaced by ``footprint``.? I think @jni would know...
There was a problem hiding this comment.
I'm pretty sure that we don't use the .. versionchanged:: directive consistently (yet). But I could go through and update each docstring if we deem it worth it to keep the notice around.
Automatic detection of the color channel based on the old deprecated `multichannel=None` was broken in version 0.19. This commit recovers the old behavior for 0.20 using a proxy object `ChannelAxisNotSet`. The behavior is still deprecated though and should be removed completely in 0.21. Note that the warning is only raised if the automatic behavior deviates from the behavior that will be the new default in 0.21, which is `channel_axis=None`. This way we only notify users that were affected by the broken behavior in 0.19 and now deprecated behavior in 0.20.
in `regionprops` (since 0.16) and `active_contour` (since 0.18).
Instead of raising a ValueError, since f8dfea6 [1] this is addressed by returning `False`. [1] scikit-image#6453
# Conflicts: # skimage/restoration/_denoise.py # skimage/segmentation/tests/test_random_walker.py
|
The one CI failure was due to an uncaught FutureWarning message. I pushed a commit that should fix it. |
| return f"<{cls.__name__}>" | ||
|
|
||
|
|
||
| class ChannelAxisNotSet(metaclass=_PatchClassRepr): |
There was a problem hiding this comment.
I'm sure there are other ways to control how this signal object is rendered in the signature (I would post the link but strangely the pipeline did not build the docs?). I initially included the __repr__ in this class itself which requires creating an instance of it in the same scope to check for identity in the function body. Compared to that, I actually found the metaclass-based approach clearer and easier to document. The class itself clearly is the signal and there is no confusion about a second symbol that also looks like a signal; it's clearly a metaclass only responsible for modifying the representation.
It's not something I feel strongly about, though. The verbose answer is mainly because I'm rationalizing my gut feeling about which approach "felt better" in retrospective. 😄
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
|
I think this should be good to go now. Some of these deprecations (e.g. |
|
I'm in favor of merging. |
|
I'm also in favour of merging this PR. |
|
Merged, thanks @lagru! |
The `multichannel` field in `skimage.util.montage` has been deprecated in version 0.20. Update function call to work with the new API. From the relase notes for 0.20: Remove deprecated parameter multichannel; use channel_axis instead. [...] In skimage.util, this affects montage and apply_parallel ([#6583](scikit-image/scikit-image#6583)).
The `multichannel` field in `skimage.util.montage` has been deprecated in version 0.20. Update function call to work with the new API. From the relase notes for 0.20: Remove deprecated parameter multichannel; use channel_axis instead. [...] In skimage.util, this affects montage and apply_parallel ([#6583](scikit-image/scikit-image#6583)).
Description
Complete deprecations that target 0.20. This PR addresses deprecations which were created for 0.19 with target 1.0. At that time a release of 0.20 wasn't planned. See also https://discuss.scientific-python.org/t/559.
Probably easiest to check the changes commit by commit (I tried to keep related changes together / rebased and squashed related changes).
multichanneldeprecation & removedeprecate_multichannel_kwargdecorator class*_iter->*_num_iterinunsupervised_wienermorphology.rectangleChecklist
./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py.doc/release/release_dev.rst.example, to backport to v0.19.x after merging, add the following in a PR
comment:
@meeseeksdev backport to v0.19.xrun-benchmarklabel. To rerun, the labelcan be removed and then added again. The benchmark output can be checked in
the "Actions" tab.