`colorsys` returning out of bounds values

[FFY00]

Bug report

On certain inputs, the colorsys module returns values that are out of the documented allowed range, see GH-100754 for the previous discussion.

Your environment

• CPython versions tested on: 3.12
• Operating system and architecture: any

Linked PRs

• gh-102581: uniform clipping rules and introduces clip argument #106254
• gh-102581: Add clipping in colorsys.py #149572

[FFY00]

I think the first thing to do here is to validate that our conversion logic is actually correct. If it is, then that means that for the given input value, there's no valid output.

In such case, I'd propose we raise a warning when the result goes out of bounds, and add a new parameter to the function(s) to clip the value, as that's what most users will want top do. Opting in to the clipping behavior makes sure we don't break anything, and still provides a reasonable solution for users that are okay with a result approximation.

I'm not sure if I am being too defensive here or not 😅

[FFY00]

@KelSolaar, you maintain https://github.com/colour-science (which is awesome!), do you have any input regarding this issue?

I am not a color theory expert by any means, so I am really not equipped to analyze the technical part of this.

[tacaswell]

This looks like it falls in the category general float errors (e.g. 0.1 * 3 != .3) to me.

Skimming back through the linked issues I could not find an example input that caused this problem, but I'm going to guess it is something at edges of the input bounds that, due to imprecision, is giving an answer just outside of output bounds.

I think just clipping is reasonable as you should be clipping away numeric issues and I can not think of a case on the caller side of this where I would want the error to come back to me!

That said, because these functions do not validate the input

>>> import colorsys
>>> colorsys.rgb_to_hls(5, 5, 5)
(0.0, 5.0, 0.0)
>>> colorsys.rgb_to_hls(500, 500, 500)
(0.0, 500.0, 0.0)
>>>

so clipping the output would do surprising things for users passing in values that are out of range.

I would advocate for adding a warning if the input is out of bounds and unconditionally clipping the output.

[ppwwyyxx]

Agree with @tacaswell that we should:
(1) Warn or raise error if input is out of bound
(2) Unconditionally clip the output.

(2) is reasonable because, we can mathematically prove that, the output must be in bound as long as the input is in bound. Since callers are responsible for providing in-bound inputs (checked in (1)), the only reason why outputs may be out of bound is the limited floating point precision. In that case clipping will simply make the results more correct.

Here is a test case I manually OCRed from #100754:

import colorsys
r = 0.6439640003843092
g = 0.2082993986149012
b = 1.0
print(colorsys.rgb_to_hls(r, g, b))
(0.7583815988449848, 0.6041496993074507, 1.0000000000000002)

[KelSolaar]

Hi,

@FFY00: Thanks for dragging me in!

There is no good reason why it should be systematically clipped. RGB values are either:

1. Bounded, typically for standard dynamic range, i.e. SDR 8, 10, 12 and 16-bit encodings with minimum representable value of 0 and maximum representable values of 255, 1023, 4095, 65535 respectively. They are also encoded with a non-linear transfer function, e.g. power (gamma) or log. Once decoded and normalised to floating point representation, they will be in range [0. 1].
2. Unbounded, typically for high dynamic range, i.e. HDR 16-half, 32-bit, 64-bit and 128-bit float. The minimum and maximum representable values are more complicated to compute, e.g. [-65504, 65504] range for 16-half. They are encoded linearly and can represent either relative of absolute values.

With that in mind, in the film industry where I work, we pretty much exclusively work with the second case and HDR imagery thus the following behaviour is totally expected and desirable to us:

>>> import colour
>>> colour.RGB_to_HSL([1500, 2500, 3000])
array([  5.55555556e-01,  -3.33481547e-01,   2.25000000e+03])
>>> colour.HSL_to_RGB(colour.RGB_to_HSL([1500, 2500, 3000]))
array([ 1500.,  2500.,  3000.])

As a general rule, we never clip in Colour unless it is absolutely required.

Hope that helps!

Cheers,

Thomas

[tacaswell]

Given @KelSolaar comment, I would argue that either the docs should be updated to indicate that these functions supports case 2 and drop the [0, 1] comments from the docs or the docs should make clear these functions are only meant to support case 1 and then clip.

It is my suspicion that most users who reach for colorsys (rather than a more specialized tool) are working in case 1, but I have no actual data one way or the other.

[KelSolaar]

It is my suspicion that most users who reach for colorsys (rather than a more specialized tool) are working in case 1, but I have no actual data one way or the other.

This is certainly true today, but it might not be tomorrow as HDR adoption grows, maybe a way to cut the apple in two is to change the function signature and add a clip argument that defaults to True and explain some of the reasoning, e.g. if you work with HDR then you can set to False. Note the you will probably want to “sanitize” and clip HSV/HSL value/ligthness input also for consistency.

[MonadChains]

Honestly I feel that the entire module is inconsistent. For example in the case rgb -> yiq there is no clipping:

cpython/Lib/colorsys.py

Lines 40 to 44 in 6b5166f

	def rgb_to_yiq(r, g, b):
	y = 0.30*r + 0.59*g + 0.11*b
	i = 0.74*(r-y) - 0.27*(b-y)
	q = 0.48*(r-y) + 0.41*(b-y)
	return (y, i, q)

while viceversa there is:

cpython/Lib/colorsys.py

Lines 46 to 67 in 6b5166f

	def yiq_to_rgb(y, i, q):
	# r = y + (0.27*q + 0.41*i) / (0.74*0.41 + 0.27*0.48)
	# b = y + (0.74*q - 0.48*i) / (0.74*0.41 + 0.27*0.48)
	# g = y - (0.30*(r-y) + 0.11*(b-y)) / 0.59
	r = y + 0.9468822170900693*i + 0.6235565819861433*q
	g = y - 0.27478764629897834*i - 0.6356910791873801*q
	b = y - 1.1085450346420322*i + 1.7090069284064666*q
	if r < 0.0:
	r = 0.0
	if g < 0.0:
	g = 0.0
	if b < 0.0:
	b = 0.0
	if r > 1.0:
	r = 1.0
	if g > 1.0:
	g = 1.0
	if b > 1.0:
	b = 1.0
	return (r, g, b)

I would propose to uniform it either by making all functions working for unlimited color space with the option to clip to remain in the space [0,1] or agree to a unique way of handling overflowing values (throwing a warning or clipping).

@KelSolaar in the case of unbounded RGB values, are the conversions to the other representations available in the module (YIQ, HLS, HSV) reversible (i.e. applying the inverse transformation should return the initial unbounded value)?

[KelSolaar]

Hello,

I would propose to uniform it either by making all functions working for unlimited color space with the option to clip to remain in the space [0,1] or agree to a unique way of handling overflowing values (throwing a warning or clipping).

I would certainly agree with that.

@KelSolaar in the case of unbounded RGB values, are the conversions to the other representations available in the module (YIQ, HLS, HSV) reversible (i.e. applying the inverse transformation should return the initial unbounded value)?

I don't really understand why rgb_to_yiq does not clip why the inverse path does not. All the 3 transforms should be invertible and the unit tests should try to test the roundtrip ideally if not already.