gh-106498: Fix an extreme case in colorsys.rgb_to_hls#106530
Closed
gaogaotiantian wants to merge 2 commits intopython:mainfrom
Closed
gh-106498: Fix an extreme case in colorsys.rgb_to_hls#106530gaogaotiantian wants to merge 2 commits intopython:mainfrom
colorsys.rgb_to_hls#106530gaogaotiantian wants to merge 2 commits intopython:mainfrom
Conversation
Member
Author
|
@terryjreedy as the reviewer of the related PR, could you share thoughts on this matter? Thanks! |
nedveder
approved these changes
Jul 8, 2023
nedveder
left a comment
There was a problem hiding this comment.
minc == maxc and sumc == 2.0 are checking for the same thing - preventing division by zero. Another possible fix would be to check if l==1 on line 91.
Member
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.