Rename img_as_* functions to rescale_to_*#7697
Rename img_as_* functions to rescale_to_*#7697lagru wants to merge 17 commits intoscikit-image:mainfrom
img_as_* functions to rescale_to_*#7697Conversation
I opted for `rescale_to_int16` and `rescale_to_uint16`, since that makes the bit-depth a lot more obvious at a first glance.
including all public places where they could be imported from.
to enable easy backwards compability.
Not sure if this is the right call. One one hand we don't want to advertise these anymore. But technically not including them might break users who do `from skimage.util import *`.
| # ========================== | ||
|
|
||
| data = ski.util.img_as_float(ski.data.cells3d()[:, 1, :, :]) # grab just the nuclei | ||
| data = ski.util.rescale_to_float(ski.data.cells3d()[:, 1, :, :]) # grab just the nuclei |
There was a problem hiding this comment.
In places where rescale_to_float* functions are seen by users, I checked whether setting legacy_float_range=True is necessary. In this case, cells3d() returns a an uint8-array. So this parameter has no effect (it's only meaningful for signed ints).
| testing.assert_allclose(img2, r2.astype(bool)) | ||
|
|
||
| img3 = img_as_float(img) | ||
| img3 = rescale_to_float(img, legacy_float_range=True) |
There was a problem hiding this comment.
In cases like this, legacy_float_range=True wouldn't be necessary, img is boolean, but for internal stuff I chose to be on the safe side and just add legacy_float_range=True. That was a lot faster than me going through every case and trying to determine if the input could be an signed integer, and potentially missing an edge case and introducing an unnecessary error.
👍 How about |
|
ubyte -> uint8? |
Can do. (Edit see 785d728)
|
stefanv
left a comment
There was a problem hiding this comment.
Some minor comments; but overall looks good to me.
I like your suggestion to stick with numpy conventions!
skimage/util/dtype.py
Outdated
| By default and if ``False``, the contents of integer images will be | ||
| scaled to the range [0.0, 1.0] if the target `dtype` is floating point. | ||
| However, if legacy behavior is enabled, images with signed integers | ||
| will be scaled to [-1.0, 1.0] instead. This parameter as no effect on |
There was a problem hiding this comment.
Why does the parameter not affect floating point images? I suppose because, if they already include negative values, they go to [-1, 1], otherwise to [0, 1]?
There was a problem hiding this comment.
Because the rescaling only happens if a conversion from int is necessary. Floats are considered fine and just the dtype is potentially changed. E.g.
ski.util.rescale_to_float64(np.array([-10, 10], dtype=float))
# array([-10., 10.])
ski.util.rescale_to_float64(np.array([-10, 10], dtype=np.float32))
# array([-10., 10.])At least that's the same behavior as the img_as_float* functions have.
There was a problem hiding this comment.
Yeah, I found that a bit weird too. We could totally change this in a follow-up PR? E.g. make legacy_float_range=True preserve the old weird behavior and make the default always scale between [0, 1] regardless of input dtype.
There was a problem hiding this comment.
On the new function, that should certainly be the case.
There was a problem hiding this comment.
I've made a first attempt at addressing this in fc09911. The docstrings probably need more polishing. But have a look. I'm curious what you think of the edge case handling (inf -> NaN, uniform -> 0) in _normalize_float_0_to_1.
|
One other observation: we rely on this flag in our test suite. What do we do when we remove that flag? It would be good to get the test suite in good enough shape not to need this flag anywhere. Then again, since the default range changes in 2.0, perhaps all those test outcomes change as well, and we're fine. Just worth checking. |
Then we pay extra close attention. In most cases it should be pretty obvious what dtype goes in. I mainly didn't do it in this PR as to not delay it even further. But could be a follow-up PR... |
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
stefanv
left a comment
There was a problem hiding this comment.
One discussion to wrap up, but I think this is good to go?
skimage/util/dtype.py
Outdated
| By default and if ``False``, the contents of integer images will be | ||
| scaled to the range [0.0, 1.0] if the target `dtype` is floating point. | ||
| However, if legacy behavior is enabled, images with signed integers | ||
| will be scaled to [-1.0, 1.0] instead. This parameter as no effect on |
There was a problem hiding this comment.
On the new function, that should certainly be the case.
| raise NotImplementedError("phase_cross_correlation unavailable") | ||
| shifts = (-2.3, 1.7, 5.4, -3.2)[:ndims] | ||
| phantom = img_as_float(data.binary_blobs(length=image_size, n_dim=ndims)) | ||
| phantom = rescale_to_float(data.binary_blobs(length=image_size, n_dim=ndims)) |
There was a problem hiding this comment.
With the update, this now always scales float arrays to the target range [0, 1]. Still need to check if this affects any of these gallery examples in a negative way.
|
Pyodide test failures are related. |
|
@lagru For consistency, can we use only |
| out -= float_min / float_ptp | ||
|
|
||
| except FloatingPointError: | ||
| if float_ptp == 0: |
There was a problem hiding this comment.
Maybe better to do an early exit without doing anything if float_ptp is zero.
|
See my comment in #1234 (comment). Closing this for now, while we resolve that discussion on which direction we want to take this. |
Description
according to the skimage2 roadmap. The new functions are no longer available in our top-level
skimagemodule as a welcome side effect.This also proposes
img_as_int(torescale_to_int16) andimg_as_uint(torescale_to_uint16).legacy_float_rangeto preserve the old behavior during the deprecation period.Closes #1234.
Checklist
./doc/examplesfor new featuresRelease note
For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.