Deprecate output and positional args in filters.gaussian#7225
Deprecate output and positional args in filters.gaussian#7225soupault merged 13 commits intoscikit-image:mainfrom
filters.gaussian#7225Conversation
Addresses an item in our skimage2 transition list [1]. We can't simply use deprecate_kwarg or remove_arg, as `output` can also be passed as a positional argument. Instead, deprecate passing more than 1 positional argument and at the same time, refactor `output` to `out`. I'm not really sure whether remove_arg is a user friendly way to deprecate a positional parameter. Applying the function prevents positional arguments after the deprecated one as well, even though this is not deprecated. The warning will users also not tell them about it. I find it clearer to just deprecate these positional arguments in favor of keyword-only ones. I also think it's the better practice. [1] https://github.com/scikit-image/scikit-image/wiki/API-changes-for-skimage2
|
Thanks for the review @mkcor. I am currently working on merging and updating our After that I'll address your feedback and update this PR accordingly. |
according to code review. Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
|
@mkcor, sorry for the "Marked as resolved / unresolved" spam, I got confused. 🙈 This should be ready to review / merge. |
|
@scikit-image/core, this would be good to prioritize as a step towards skimage2. 🙏 |
According to [1], deprecations should be inplace for two releases. If merged this will be releases 0.23 and 0.24. So 0.25 will finalize the deprecation. [1] https://scikit-image.org/docs/dev/development/contribute.html#deprecation-cycle
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
as suggested by Marianne. I felt like "got" conveyed the same meaning as "provided" but is a bit less advanced vocabulary.
which was recently merged.
|
Thank you, Lars!
If it is still planned for the current branch, I think it would be acceptable to have both deprecations running at the same time. The implementation could then count the number of arguments passed positionally (e.g. as in https://stackoverflow.com/questions/67639234/count-how-many-arguments-passed-as-positional). No comments to the PR otherwise, and I am happy to proceed with the current version. |
|
Good call @soupault. I forgot to update the original intention in the PR description. I originally wanted to turn I'd be happy if we turned that parameter into a keyword-only one but think it would be reasonable to do it in another PR perhaps? |
|
Sure. Thanks for working on this! |
Description
Addresses an item in our skimage2 transition list. We can't simply use
deprecate_kwargorremove_arg, asoutputcan also be passed as a positional argument. Instead, deprecate passing more than 1 positional argument and at the same time, refactoroutputtoout.I'm not really sure whether
remove_argis a user friendly way to deprecate a positional parameter. Applying the function prevents positional arguments after the deprecated one as well, even though this is not deprecated. The warning will users also not tell them about it. I find it clearer to just deprecate these positional arguments in favor of keyword-only ones. I also think it's the better practice.Checklist
./doc/examplesfor new featuresRelease note
Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically: