Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Complete deprecations targeting release 0.20 or 1.0 #6583
Complete deprecations targeting release 0.20 or 1.0 #6583
Changes from 17 commits
7430c41
d951774
64e0023
198566f
6db637d
50cdfb0
4cb8866
56f0431
f77d0da
171d34a
9b2c8f0
c7337b6
c25941e
1beab4a
6a24f27
b53b46a
38c23c4
998ee00
a5421d5
d752adf
206e599
bc476b7
9b70ed6
e7d8c75
15638d4
cc32ae2
dc843fe
246bc46
529c74a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 aschannel_axis=None
is replaced withchannel_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 tochannel_axis
we can no longer replicate this behavior.channel_axis=None
means "no RGB" as opposed tomultichannel=None
which means "not set".Pretty sure this was broken since the
deprecate_multichannel_kwarg
was applied as well. A that one translatesmultichannel=False
intochannel_axis=None
...My proposal is to remove this (broken) implicit RGB assumption if
(M, N, 3)
.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need another None-proxy, e.g.:
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.
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:
multichannel
norchannel_axis
and 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.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 becausemultichannel
is no longer available.channel_axis=None
for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefanv @lagru What is the status of this? It would be good to get this merged soon.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for patching up this behavior @lagru, it looks great.