python / cpython

The Python programming language
https://www.python.org
Other
63.38k stars 30.35k forks source link

simplify `colorsys.rgb_to_hsv` #124879

Open Yay295 opened 1 month ago

Yay295 commented 1 month ago

Feature or enhancement

Proposal:

By inlining some variables, the number of operations performed can be reduced. Example: with

gc = (maxc-g) / rangec
bc = (maxc-b) / rangec

then

h = bc-gc
h = (maxc-b)/rangec - (maxc-g)/rangec
h = ((maxc-b) - (maxc-g)) / rangec
h = (maxc - b - maxc + g) / rangec
h = (g-b) / rangec

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

Yay295 commented 1 month ago

Another optimization could be done here: https://github.com/python/cpython/blob/9ce90206b7a4649600218cf0bd4826db79c9a312/Lib/colorsys.py#L142

n % 1 is the same as n - floor(n). However, this technically changes the function argument requirements from needing the __mod__ or __rmod__ methods to needing the __floor__ method.

terryjreedy commented 1 month ago

Please look at the rejected issue #114577 and PR #114578 to see if any of the comments are relevant here.

Yay295 commented 1 month ago

They are not. That PR did not functionally change colorsys.rgb_to_hsv, but it did replace some ifs with function calls elsewhere, which my change does not do.

Wulian233 commented 1 month ago

Please look at the rejected issue #114577 and PR #114578 to see if any of the comments are relevant here.

I remember that😂 My first PR and rejected