Open idc9 opened 6 months ago
Thanks for the bug report. Indeed this is likely a confusing part of our API.
Perhaps an improvement to the documentation would suffice?
You can make a PR making modifications to https://github.com/scikit-image/scikit-image/blob/21485b18ddd21e44b1881a34e64b2ec4d5feee4e/skimage/transform/_warps.py#L833
And the added explination will appear in jupyter notebooks and our online documentation
It might be worth changing the behavior of warp
instead of changing the documentation. The options I see are
Requiring the user to manually scale cval might not be a good idea. It may not occur to many people to do this. Also it's a little bit involved to get the scaling behavior correct. The solution I'm currently using is
cval_maybe_rescaled = img_as_float(np.array([cval]).astype(image.dtype)).item()
which is a little bit ugly/involved for the user to do on their own.
I would vote for option 1, but it might be worth getting other people's input on this.
Option 1
warp
will handle possibly rescaling. Option 2
Option 2, default to automatic rescale
Option 2, default to current behavior with no rescale
Option 3
warp
depending on what happens inside of warp
. If we go with option 2/3 we could add a function to skimage def rescale_cval_to_float(cval, image)
that handles rescaling the cval.
I don't disagree with any of the core concepts you bring up, i'm just not sure a 1-2 year(+???) deprecation cycle is really what you want. The worst part for you, will likely be that you will have old code that requires an old version of scikit image (for whatever reason) that will just behave exactly in the way you do not expect. You'll end up writing your own compatibility shims to work with scikit-image 0.20.0 (1 year old) to today's version (0.22.0) through to the future versions where your proposals would end up being the default.
preserve_range
and units were a big challenge that we ultimately weren't able to resolve quickly enough (well 6 years now)
https://github.com/scikit-image/scikit-image/issues/3263
I'm trying to find where consensus along the core maintainers (myself included) has landed and the general idea is to set preserve_range=True
everywhere and make that the only option in skimage(next). Ultimately the problem is that with large datasets, that floating point conversion is just costly in terms of computation, and often times unexpected. Casting to float32 is might be costly, but the value of doing / 255
just to rescale again by some contrast adjustment code is often just silly.
This is one comment that hints at that, but we had a larger discussion somewhere https://discuss.scientific-python.org/t/a-pragmatic-pathway-towards-skimage2/530/20
Option 1 and 2 are fine proposals, just given the bandwidth of the core maintainers, I don't think they will get implemented.
The line between bug and feature is quite thin in this particular example, unfortunately, documenting this quirk in the behavior might be the fastest and most effective approach for all parties.
I concur with @hmaarrfk.
In a vacuum option 1 seems most intuitive to me. However, it also seems like something that changes output for the same input. We have a policy not to do that. Even if an intermediate deprecation would warn most users, some might miss it and might have trouble reproducing their results down the line. As Mark hinted at, we plan to remove automatic scaling in most cases by default in "skimage2". I'll add this as a problem to address in API changes for skimage2.
In the meantime and in addition to documenting the behavior, we could raise a warning if the cval
is outside the value range of the rescaled image. If you would like to do a PR for this @idc9, feel welcome to request my review on it. :D
Got it -- that all makes sense. I'll put in a PR when I get a chance to come back to this!
Hello scikit-image core devs! There hasn't been any activity on this issue for more than 180 days. I have marked it as "dormant" to make it easy to find. To our contributors, thank you for your contribution and apologies if this issue fell through the cracks! Hopefully this ping will help bring some fresh attention to the issue. If you need help, you can always reach out on our forum If you think that this issue is no longer relevant, you may close it, or we may do it at some point (either way, it will be done manually).
Not stale
Description:
When you call warp with
preserve_range=False
. The values of the pixels in are usually scaled (e.g. if the input image were uint8 then 255 gets mapped to 1). Howeverwarp
does not also scale thecval
which is used to pad the image.In the example below the pixels in the non-padded part of the output image are all between 0 and 1, but the padded pixels are set to 255.
I think the most straightforward solution is to rescale cval if
preserve_range=False
.Way to reproduce:
1.0
255.0
Version information: