Skip to content
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

warp does not scale cval leading to unexpected (wrong?) behavior with preserve_range=False and non-zero cval #7336

Open
idc9 opened this issue Mar 8, 2024 · 5 comments
Labels
🐛 Bug 📄 type: Documentation Updates, fixes and additions to documentation

Comments

@idc9
Copy link

idc9 commented Mar 8, 2024

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). However warp does not also scale the cval 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:

from skimage import data
import numpy as np
import matplotlib.pyplot as plt

from skimage.transform import SimilarityTransform, warp
from skimage.color import rgb2gray


# uint8 grayscale image
image = data.astronaut()
image = rgb2gray(image)
image = (255 * image).astype(np.uint8)

tform = SimilarityTransform(translation=[20, 0])

# default with cval =0 -- the scaling problem does not show up here
wapped = warp(image=image, 
              inverse_map=tform.inverse,
              preserve_range=False,
              cval=0 # deafult value
             )

# all values in the image are between 0 and 1 since preserve_range=False
print(wapped.max())

1.0

plt.figure()
plt.imshow(wapped)
Screenshot 2024-03-07 at 7 12 06 PM
# now set non-zero cval
wapped = warp(image=image, 
              inverse_map=tform.inverse,
              preserve_range=False,
              cval=255 #
             )

# the padded values are 255, which was not scaled
print(wapped.max())

255.0

plt.figure()
plt.imshow(wapped)
Screenshot 2024-03-07 at 7 11 01 PM

Version information:

3.8.18 (default, Sep 11 2023, 08:17:33) 
[Clang 14.0.6 ]
macOS-10.16-x86_64-i386-64bit
scikit-image version: 0.21.0
numpy version: 1.24.4
@idc9 idc9 added the 🐛 Bug label Mar 8, 2024
@hmaarrfk
Copy link
Member

hmaarrfk commented Mar 8, 2024

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

Used in conjunction with mode 'constant', the value outside

And the added explination will appear in jupyter notebooks and our online documentation

@hmaarrfk hmaarrfk added the 📄 type: Documentation Updates, fixes and additions to documentation label Mar 8, 2024
@idc9
Copy link
Author

idc9 commented Mar 9, 2024

It might be worth changing the behavior of warp instead of changing the documentation. The options I see are

  1. Change behavior so cval is automatically rescaled if the image is rescaled.
  2. Add an argument to give user the option to either automatically convert cval or to always preserve the input cval. Need to decide option would be the default behavior.
  3. Keep the current behavior where the cval is not changed even if the image values are. Change the documentation to make user aware of this issue.

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.

Tradeoffs

Option 1

  • pro: the user can safely enter the cval in the original scale and warp will handle possibly rescaling.
  • pro: no thought on the user's side, not option for user to do the wrong thing
  • con: breaking change

Option 2

  • con: introduces an additional argument
  • pro: keep option for old vs. new behavior (not sure if this is usful?)

Option 2, default to automatic rescale

  • pro: keeps pros of option 1
  • con: breaking change

Option 2, default to current behavior with no rescale

  • pro: no breaking change
  • con: default option results in wrong/unintuitive behavior of cval

Option 3

  • pro: no breaking change
  • con: user has to manually scale the cval before passing into 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.

@hmaarrfk
Copy link
Member

hmaarrfk commented Mar 9, 2024

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)
#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.

@lagru
Copy link
Member

lagru commented Mar 10, 2024

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

@idc9
Copy link
Author

idc9 commented Mar 13, 2024

Got it -- that all makes sense. I'll put in a PR when I get a chance to come back to this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug 📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants