astropy / imageutils

Image processing utilities for Astropy (No longer maintained)
https://imageutils.readthedocs.org/
9 stars 17 forks source link

Documentation for ``find_imgcuts`` confusing #16

Closed astrofrog closed 9 years ago

astrofrog commented 9 years ago

I haven't tried using it yet, but am a bit confused by

https://imageutils.readthedocs.org/en/latest/api/imageutils.find_imgcuts.html#imageutils.find_imgcuts

Why are min_cut and max_cut needed as inputs since it is what is returned? There are mentions of scaling the image, but the image should remain unchanged, correct?

astrofrog commented 9 years ago

Also, for returns, it should not say out but instead the outputs should be min_cut and max_cut. All functions that return multiple arguments return a tuple, so I don't think that needs to be specified.

astrofrog commented 9 years ago

I realize now that it's because there is a template docstring (but I think it should still be fixed).

As a side note, is there really a need to have one function per scaling? Couldn't this be done by a single function, preventing the need to have the docstring being copied? In APLpy we have a normalize class that matplotlib uses:

https://github.com/astrofrog/aplpy/blob/master/aplpy/normalize.py

Of course this is matplotlib-specific so I'm not suggesting doing that, but it shows you don't need that many arguments if you have a single point of entry. We also have pmin and pmax defined elsewhere that the user can use to define the percentile. Anyway I'd be happy at some point to try and work on refactoring this if there was interest (and I'm hoping APLpy can then just simply use imageutils for the scaling).

larrybradley commented 9 years ago

Thanks, @astrofrog. Yes, as I mentioned in https://github.com/astropy/imageutils/pull/11#issuecomment-51109051, I'm going to combine them in one function with a keyword option specifying the type of scaling. That looks similar to what you did in APLpy. Is the name stretch preferable to scaling?

I don't recall why I left min/max_cut as optional inputs (it may have been to have one single function parse all the "cutlevel" inputs and return the cut levels without having to repeat parsing the inputs). It's not really needed since min/max_cut could be parsed out in rescale_img and then send only the "percentage" keywords on to find_imgcuts. That would be less confusing.

There are mentions of scaling the image, but the image should remain unchanged, correct?

I'm not sure what you mean by this. find_imgcuts doesn't scale anything. rescale_img (and the scale_* functions) rescales the image between 0 and 1 (using the min/max cuts). The image will be unchanged only if its min/max was 0 and 1 and linear scaling is used.

astrofrog commented 9 years ago

@larrybradley - regarding my comment on scaling, the issue is that e.g. percentile says 'The percentage of the image values to scale.' in find_imgcuts, but of course, find_imgcuts doesn't scale anything.

larrybradley commented 9 years ago

Ah. Yes, that's because of using the template docstring (now I know what you meant by that). I will fix that in the new version. Thanks.

larrybradley commented 9 years ago

This was addressed in #17, so I'm closing.