Repository navigation
colorsys.rgb_to_hls raises ZeroDivisionError on nearly-white inputs #106498
Description
Bug report
In Python 3.10 and 3.11, the colorsys.rgb_to_hls function divides by zero when given a valid color which is very close to but not exactly pure white.
Python 3.11.1 (main, Feb 8 2023, 15:39:14) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import colorsys
>>> colorsys.rgb_to_hls(1, 1, 0.9999999999999999)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/dfremont/.pyenv/versions/3.11.1/lib/python3.11/colorsys.py", line 86, in rgb_to_hls
s = rangec / (2.0-sumc)
~~~~~~~^~~~~~~~~~~~
ZeroDivisionError: float division by zeroThe current code assumes that, given 3 numbers between 0 and 1, if their minimum and maximum are not equal, then the sum of the minimum and maximum must be strictly less than 2. That claim is true for real numbers but not for floating-point arithmetic. The example above fails because 1 + 0.9999999999999999 == 2.0.
Your environment
- CPython versions tested on: 3.8.8 (works); 3.9.13 (works); 3.10.3 (buggy); 3.11.1 (buggy)
- Operating system and architecture: macOS 12.6.6; x86_64
Activity
gaogaotiantian commented on Jul 7, 2023
Very interesting and super extreme bug. I don't even know if this should be fixed. In my opinion, "correct" in super extreme case is more important than "very slight performance improvement", but this code has not been touched for a while.
The fix is simple, but will hit performance slightly. We should wait for a core dev to make the decision whether to fix it.
dfremont commented on Jul 7, 2023
Thanks, Tian. It's certainly a corner case, but it came up naturally in our project: our code generates random colors, and one of our CI tests failed because we got unlucky and hit this bug. We have to work around it, since it could happen again, so we either need to check for the exception (which is what we're currently doing in Scenic-Foundation/Scenic#153) or implement a correct algorithm ourselves. I'd much rather have the colorsys function be correct but slightly slower.
terryjreedy commented on Jul 11, 2023
As @gaogaotiantian noted on PR #106530, the regression is due to issue #86618 and PR #22306, which applied an optimization that was incorrect in one place, by assuming that 2.0 - maxc - minc == 2.0 - (maxc + minc). This is true mathematically, but not for floats in the example case.
The regression should definitely be fixed, and the correct fix is to undo the incorrect replacement on line 86. The result is 6 different output for the 6 possible cases with maxc = 1.0 and minc = 0.9999999999999999.
Replacing the 6 outputs with (0, 1, 0) is incorrect, or at least a change from those prior to #86618. That issue was not intended to make any such change. Note that hls_to_rgb(h, 1, 1) is (1, 1, 1) regardless of h so the outputs above do not roundtrip.
@dfremont I will backport the reversion to 3.11 but cannot do so for 3.10, which now only gets security fixes.
pythongh-106498: Revert incorrect colorsys.rgb_to_hls change
gh-106498: Revert incorrect colorsys.rgb_to_hls change (#106627)
pythongh-106498: Revert incorrect colorsys.rgb_to_hls change (pythonG…
pythongh-106498: Revert incorrect colorsys.rgb_to_hls change (pythonG…
[3.12] gh-106498: Revert incorrect colorsys.rgb_to_hls change (GH-106627
[3.11] gh-106498: Revert incorrect colorsys.rgb_to_hls change (GH-106627
gaogaotiantian commented on Jul 11, 2023
@terryjreedy I know you've finished the patch, and the patch works properly. Just want to mention that the reason to reject the orignal patch is not valid - in hls space, all (x, 1.0, y) are the same color - white. So the six "different" results are the same. There's no extra benefit to have different "white"s in this case. As you mentioned - the reverse won't work, because they are literally the same color, and it will only convert back to (1, 1, 1) - which is white.
gaogaotiantian commented on Jul 12, 2023
Actually, having another thought at this issue, I think the current test should be improved.
As I mentioned, any (x, 1.0, y) is equivalent - they are just white. Testing the result with assertTripleEqual is actually testing a very implement-specific behavior, not the validity of the algorithm. Say in the future, we come up with some "better" algorithm (assuming there is one), which converts rgb(1.0, 1.0, 0.9999999999999999) to hls(0, 1, 1), which would be totally correct and valid, but would make our tests fail.
For near white colors, we should either just make sure they do not raise exceptions, or test the l value is 1.0 - the checking for hue and saturation is only for implmenetation.
terryjreedy commented on Jul 12, 2023
Please read again "Replacing the 6 outputs with (0, 1, 0) is incorrect, or at least a change from those prior to #86618. That issue was not intended to make any such change." In 3.10 I introduced a regression in this edge case - a unintended division by 0. This issue is about undoing that. Period. The simplest change is to undo the mistake that caused the problem.
Changing the output from what it was in 3.9 might also break code, perhaps the OP's regression test, though in a different way. The reason this module was kept after it was proposed to remove it was that some people legitimately used it and it was supposedly stable and low maintenance. Changing the output to another 'equivalent' (but definitely not equal) output would need a real justification on a new issue. A different algorithm that changed output should break a test. Changing a test requires explanation.
gaogaotiantian commented on Jul 12, 2023
Okay I understood. If we need to keep the behavior the same as before #86618, then this is the way to fix it. And sure if this module is in maintanence mode, we should make as few behavior changes as possible.