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

Explicitly cast the input array to float before doing anything to it #2569

Merged
merged 2 commits into from Dec 12, 2013

Conversation

tacaswell
Copy link
Member

so that the computation of s does not compute as integer division.

fixes issue #2566

@@ -1231,6 +1231,7 @@ def rgb_to_hsv(arr):
convert rgb values in a numpy array to hsv values
input and output arrays should have shape (M,N,3)
"""
arr = arr.astype('float')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding the copy=False kwarg so that a copy is made only if necessary. Also, ideally, this cast should be done only if the input is not already float32 or float64.

@tacaswell
Copy link
Member Author

Once people are happy with this I will squash these commits.

@efiring
Copy link
Member

efiring commented Nov 2, 2013

Looking more closely, one sees that the problem in #2566 is not just a matter of float versus int; it is that when the image comes in as rgb or rgba uint8, as it does when a jpeg is read by imread, the range of numbers is of course 0 to 255, not 0.0 to 1.0. Therefore the rgb_to_hsv algorithm does not handle it correctly regardless of whether it is cast to float or not. rgb_to_hsv and its inverse should detect the two possible cases--float in the 0-1 range, or uint8 in the 0-255 range--and handle them separately as appropriate, returning an output that matches the dtype and range of its input. I don't know whether there is a perfect inverse pair of integer algorithms for handling the integer case. If not, then an approximation going through float would be needed. As a sidelight to that, it might make sense to use float32, not float64, for any such operations so as to reduce the memory requirement, in case the operation is being done on a very large image or set of images. Similarly, for handling float inputs, if an input is float32, then ideally the internal calculations and the output should also be float32 for this pair of functions.

@tacaswell
Copy link
Member Author

@efiring Can you take a look at this again? We talked about this in the hang out an the conclusion was this function should only support data in the [0, 1] range.

Turns out that is also what the file says at the top.

@mdboom
Copy link
Member

mdboom commented Nov 18, 2013

Yeah, I think the action items here are:

  • document in the docstring of rgb_to_hsv that the input is in range 0-1 (though it is in the top of the file as well, that should make it more obvious.) It may make sense to add that to other docstrings in this file as well.
  • I think silently casting to float here (as this PR does) is probably correct then, since failing to do so breaks the subsequent math.
  • I don't think range checking the input is necessarily a good idea for performance reasons -- documenting the expected input should hopefully be enough.
  • The test in this PR should be update to use input in the range 0-1.

# check dimensionality
if len(arr.shape) != 3:
raise ValueError(("Input array much have " +
"dimention 3 not {}".format(len(arr.shape))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling of "dimension".
I suggest using arr.ndim instead of len(arr.shape).
I also prefer the good old percent formatting for things like this. More concise, more readable.

functions now take anythang that can be coreced into a (..., 3) shaped ndarray

make documentation clear that the input must be in range [0, 1]

properly deal with integer (binary) input

added round-trip tests
@tacaswell
Copy link
Member Author

rebased and squashed.

@efiring
Copy link
Member

efiring commented Dec 1, 2013

Using the rgb_to_hsv to illustrate: the following solves several problems in your present version.

  1. Don't cause side effects by modifying input.
  2. Return the same dimensionality as you get.
  3. Improve the error message (version in this PR is actually incorrect).
import numpy as np

def rgb_to_hsv(arr):
    arr = np.asarray(arr)

    # check length of the last dimension, should be _some_ sort of rgb
    if arr.shape[-1] != 3:
        raise ValueError("Last dimension of input array must be 3; "
                         " shape {} was found.".format(arr.shape))

    # make sure we don't have an int image
    if arr.dtype.kind in ('iu'):
        arr = arr.astype(np.float32)

    ndim_in = arr.ndim
    if ndim_in == 1:
        arr = np.array(arr, ndmin=2) # makes a copy

    out = np.zeros_like(arr)

    arr_max = arr.max(-1)
    ipos = arr_max > 0
    delta = arr.ptp(-1)
    s = np.zeros_like(delta)
    s[ipos] = delta[ipos] / arr_max[ipos]
    ipos = delta > 0
    # red is max
    idx = (arr[..., 0] == arr_max) & ipos
    out[idx, 0] = (arr[idx, 1] - arr[idx, 2]) / delta[idx]
    # green is max
    idx = (arr[..., 1] == arr_max) & ipos
    out[idx, 0] = 2. + (arr[idx, 2] - arr[idx, 0]) / delta[idx]
    # blue is max
    idx = (arr[..., 2] == arr_max) & ipos
    out[idx, 0] = 4. + (arr[idx, 0] - arr[idx, 1]) / delta[idx]

    out[..., 0] = (out[..., 0] / 6.0) % 1.0
    out[..., 1] = s
    out[..., 2] = arr_max

    if ndim_in == 1:
        out.shape = (3,)

    return out

@tacaswell
Copy link
Member Author

@efiring I think I addressed your issues. This good to merge?

efiring added a commit that referenced this pull request Dec 12, 2013
Explicitly cast the input array to float before doing anything to it
@efiring efiring merged commit 80d27ee into matplotlib:v1.3.x Dec 12, 2013
@tacaswell tacaswell deleted the fix_rgb_to_hsv_2566 branch December 12, 2013 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants