Code Monkey home page Code Monkey logo

Comments (7)

lagru avatar lagru commented on September 26, 2024 2

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

from scikit-image.

hmaarrfk avatar hmaarrfk commented on September 26, 2024 1

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

from scikit-image.

idc9 avatar idc9 commented on September 26, 2024 1

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

from scikit-image.

idc9 avatar idc9 commented on September 26, 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.

from scikit-image.

hmaarrfk avatar hmaarrfk commented on September 26, 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.

from scikit-image.

github-actions avatar github-actions commented on September 26, 2024

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

from scikit-image.

hmaarrfk avatar hmaarrfk commented on September 26, 2024

Not stale

from scikit-image.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.