Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-106498: Fix an extreme case incolorsys.rgb_to_hls#106530
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
gaogaotiantian commentedJul 7, 2023
@terryjreedy as the reviewer of the related PR, could you share thoughts on this matter? Thanks! |
nedveder left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
minc == maxc andsumc == 2.0 are checking for the same thing - preventing division by zero. Another possible fix would be to check ifl==1 on line 91.
terryjreedy commentedJul 11, 2023
@gaogaotiantian Thank you for finding the change that caused the regression (which I merged, not just reviewed). As explained on this issue, the fix is incorrect. I am preparing a new PR with a different test addition. |
Uh oh!
There was an error while loading.Please reload this page.
colorsys.rgb_to_hls(1, 1, 0.9999999999999999)raisesZeroDivisionErrornow (this probably is the ONLY case). Not sure if it's worth fixing, but I think "correct" in all cases is more important than "very slightly faster in theory".This optimization is originally introduced in#23306, using the benchmark given in the PR, there's no observable performance regression with the extra check.
Benchmark Code
Another possible implementation is to do a try-except block for the division. As
tryis almost no cost now, it would be theoretically faster than the current one. The return value would be different (a near white color can be represented in many forms in hls space). I prefer the current solution as it's easy to understand with the comment, but I do not oppose the other way.