Improved gaussian filter and convert_to_float(#5195)

[michalkrawczyk]

Description

Propositon of fix #5195.
Since one point of problem, which is with not handling 1D array, was already resolved - This is mostly fix of normalizing in img_to_float() from int types according to Data Conversion Rules

Also changed preserve_range from filters.gaussian to True. The values in Gaussian Filter shoudn't be normalized by default - It will only give misleading results (Like in Issue #5195 itself)

For reviewers

• Check filters.gaussian() for 1D array ( e.g np.arange(9)), especially with preserve_range=False
• Check util.dtype.img_to_float() with some 1D, ... ,nD values
• Check if current usages of filters.gaussian() really need preserve_range==False (Optional)

[mkcor]

When updating a function's signature, please update its docstring as well (and accordingly).

[michalkrawczyk]

I quite didn't see point of doing that here. See part of current docstring:

preserve_range : bool, optional

    Whether to keep the original range of values. Otherwise, the input
    image is converted according to the conventions of ``img_as_float``.
    Also see
    https://scikit-image.org/docs/dev/user_guide/data_types.html

But maybe it should be rewritten to be more specific. If you consider that as necessery - I may do that

[mkcor]

Yes, I think it is a good opportunity to rewrite it in a clearer way: "Whether to keep the original range of values. Default is True. If False, the input image is converted according to the conventions of img_as_float, i.e., renormalised based on the range of the assumed data type: https://scikit-image.org/docs/dev/user_guide/data_types.html"

[pep8speaks]

Hello @michalkrawczyk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file skimage/segmentation/tests/test_active_contour_model.py:

Line 20:80: E501 line too long (81 > 79 characters)
Line 36:80: E501 line too long (83 > 79 characters)
Line 52:80: E501 line too long (81 > 79 characters)

Comment last updated at 2021-06-19 21:28:59 UTC

[michalkrawczyk]

A bit changed conversion at dtype since according to Direct3D most negative value should be not calculated by formula, but just changed to -1.0f. That made test to fail (Also added few more tests)

[michalkrawczyk]

It seems like some test just failed because some functions are basing at gaussian with preserve_range=False.
I actually start to wonder if that usages should be rewritten to preserve_range=False or just get back to preserve_range on default set to True and leave note at documentation.

[mkcor]

Yes, I think it is a good opportunity to rewrite it in a clearer way: "Whether to keep the original range of values. Default is True. If False, the input image is converted according to the conventions of img_as_float, i.e., renormalised based on the range of the assumed data type: https://scikit-image.org/docs/dev/user_guide/data_types.html"

[mkcor]

Since this PR changes the default value of a function's parameter, it will affect existing code. I guess we need some kind of warning message referring to the next release version, right? @scikit-image/core

[alexdesiqueira]

I guess we need some kind of warning message referring to the next release version, right?

Hi @michalkrawczyk @mkcor, please refer to our deprecation cycle policy in this case.
Thank you for your work!

[michalkrawczyk]

Done. @mkcor May I ask you to have a look now?

[jni]

Hi all, sorry to jump in really late here but I don't think we should take this leap. filters.gaussian will be one of our most used functions and will now be obnoxiously noisy. Additionally, there is a question of consistency with the rest of the library — a quick search shows that all our functions use preserve_range=False, except radon_transform and non_local_means, for which we are in the middle of a deprecation to change it to False!

In short, although I do support setting preserve_range=True everywhere, it should be done as a concerted effort in the road to 1.0, not as a piecemeal, whatever is most convenient for a specific function at a specific time approach.

[michalkrawczyk]

@jni Yeah I do see that actually almost all your current libraries use preserve_range=False.
The problem is that library requires more preserved_range=False while users requires more preserved_range=True and misleading results from False are more considered as bug.
It's not actually more convienient - it's more likely how gaussian filter should be in practice, without any 'additional features' by default.

[jni]

Indeed! As I said above I think we should make the change eventually. But when we move to 1.0 we have a chance to do it without the noise. Unfortunately we can't just change the output of the function without warning as that would break many, many users' existing code.

[grlee77]

I requested a minor change to use an existing decorator to handle the deprecation. Otherwise this looks good to me.

[michalkrawczyk]

Module cannot be found: 'skimage.restoration._inpaint' after merge with main

[grlee77]

Module cannot be found: 'skimage.restoration._inpaint' after merge with main

I think this is because we recently added a new Cython function for inpainting. After you merged main, you would have had to rebuild scikit-image in order to avoid this error.

I went ahead and ran this locally and fixed up some issues related to warnings in 528a88f. This is basically just either providing the preserve_range keyword or use expected_warnings to catch the deprecation warnings so that the test suite can run without raising any of these warnings.

The cosmetic changes in 0fe4242 are just for consistency with the style @rfezzani and I had been using in several other recent PRs.

[grlee77]

Thanks @michalkrawczyk, this looks good to me now

[grlee77]

The CI errors seen here are unrelated to this PR (I saw them in a few other cases recently as well)

[alexdesiqueira]

Thank you @michalkrawczyk!

[jni]

Hi everyone, I'm really sorry about this, but I think we should revert this. I probably should have been more explicit in my comment above: I don't think we should have this in 0.19. I think 1.0 should preserve_range=True everywhere without the keyword, and we should do that with a release-wide warning, not piecemeal warnings + kwargs everywhere. Again, the Gaussian filter is among the most used functions in scikit-image. I think it's a major mistake to force everyone using it to add a keyword argument to it that should probably be removed eventually anyway.

Since @stefanv has also expressed concerns about some of the deprecations we're making, I think we should probably have a focused meeting soon about our upcoming API choices and how we will go about making the changes.

[stefanv]

Since @stefanv has also expressed concerns about some of the deprecations we're making, I think we should probably have a focused meeting soon about our upcoming API choices and how we will go about making the changes.

Thanks for bringing this up, Juan. Would you be willing to set up that meeting and write up a paragraph or two about the challenge faced to bring everyone up to speed?

[grlee77]

Thanks for bringing this up, Juan. Would you be willing to set up that meeting and write up a paragraph or two about the challenge faced to bring everyone up to speed?

I just opened #5439 to summarize some of the things I am aware of. Many are relatively easy to implement once we have a decision on the desired direction.