matplotlib / matplotlib

matplotlib: plotting with Python
https://matplotlib.org/stable/
19.98k stars 7.56k forks source link

[ENH]: rgb_to_hsv/hsv_to_rgb could check that the inputs are of floating point dtype #22651

Open anntzer opened 2 years ago

anntzer commented 2 years ago

Problem

If one passes 8-bit RGB values to rgb_to_hsv, the results are nonsensical. rgb_to_hsv indeed explicitly states that inputs should be 0-1 floats, but it is not too difficult to accidentally pass 8-bit values instead (e.g., plt.imread() is inconsistent as to whether it returns one or the other).

Proposed solution

rgb_to_hsv could include some check for that.

timhoffm commented 2 years ago

technically one could be passing a 0-1 image with integer dtype (where all the values are 0 or 1) but this seems rare and confusion-prone anyways.

Since python 3 borders between using integer and float literals diffuse because truediv often just does the right thing if needed. colors.rgb_to_hsv((1, 0, 0)) is currently working and I'd be strongly against breaking that.

How about checking that values are in the range [-eps, 1+eps] for, e.g. eps=1e-3 and coerce to [0, 1]. That should be large enough to cover even 16bit float errors. OTOH it's small enough to likely not accept any really wrong input.

anntzer commented 2 years ago

Perhaps one could also check for the case where the input is already an ndarray (likely quite common), in which case the dtype should hopefully be better defined. No really strong opinion there, though.

timhoffm commented 2 years ago

I have the feeling that types are not too helpful here. We anyway have to define how to handle float inputs that are not in [0, 1]. E.g. how do we want to react to rgb_to_hsv((1.5, 0, 0))? IMHO that should raise an error. The only open question is whether we use hard limits [0, 1], which is technically ok. Or whether we accept [-eps, 1+eps]. With either solution I anticipate that integer inputs will automatically behave the same as their float counterparts.