gh-124879: simplify colorsys.rgb_to_hsv

[Yay295]

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

• Issue: simplify colorsys.rgb_to_hsv #124879

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[sobolevn]

Please provide performance data for both cases.

[Yay295]

before h = (maxc-b)/rangec - (maxc-g)/rangec has three subtractions and two divisions
after h = (g-b) / rangec has one subtraction and one division

[picnixz]

I think Nikita wanted more relevant benchmarks such as using timeit or pyperf. While the number of elementary operations is indeed less, I'm not sure how much performances we would gain (though it does simplify the reading IMO) (for instance, the performances would be more meaningful if you call colorsys.rgb_to_hsv quite a lot, but if you only call it once, I don't think it matters much). I however like the fact that it's easier to read (and closer to the Wikipedia's formula).

[gaogaotiantian]

One factor to consider here is that in floating point operations, mathmatically equivalent does not guarantee the same result. This is not desicive but it introduces the possibility to break someone's test. This is a general issue for all the old but not deprecated libraries - people are using them, but we are not actively developing them. We need a strong motivation to change it, and a benchmark is persuasive. If the perf improvement is not observable, we should probably not risk it.

[picnixz]

In this specific case, it might be a bit hard to say whether we gain in numerical stability or not. But yes, I think that except for the nice reading improvement, historical numerical stability is probably better (so unless we really gain a performance, we shouldn't change this code)

[Yay295]

Trying to test just this line, there is actually a noticeable improvement.

C:\Users\Yay295>py -m timeit -s "nums = [x/100 for x in range(0,100)]" "for r in nums:" "  for g in nums:" "    for b in nums:" "      maxc,minc=max(r,g,b),min(r,g,b)" "      rangec=maxc-minc" "      if rangec: (maxc-b)/rangec - (maxc-g)/rangec"
1 loop, best of 5: 450 msec per loop

C:\Users\Yay295>py -m timeit -s "nums = [x/100 for x in range(0,100)]" "for r in nums:" "  for g in nums:" "    for b in nums:" "      maxc,minc=max(r,g,b),min(r,g,b)" "      rangec=maxc-minc" "      if rangec: (g-b) / rangec"
1 loop, best of 5: 389 msec per loop

[picnixz]

Trying to test just this line, there is actually a noticeable improvement.

Can we rather have the timings for the entire function? I think it would be easier to judge. In addition, is your build a debug/release or PGO/LTO/BOLT build?

[gaogaotiantian]

Trying to test just this line

But no one calls this line only.

[Yay295]

import timeit

def rgb_to_hsv_old(r, g, b):
    maxc = max(r, g, b)
    minc = min(r, g, b)
    rangec = (maxc-minc)
    v = maxc
    if minc == maxc:
        return 0.0, 0.0, v
    s = rangec / maxc
    rc = (maxc-r) / rangec
    gc = (maxc-g) / rangec
    bc = (maxc-b) / rangec
    if r == maxc:
        h = bc-gc
    elif g == maxc:
        h = 2.0+rc-bc
    else:
        h = 4.0+gc-rc
    h = (h/6.0) % 1.0
    return h, s, v

def rgb_to_hsv_new(r, g, b):
    maxc = max(r, g, b)
    minc = min(r, g, b)
    rangec = (maxc-minc)
    v = maxc
    if minc == maxc:
        return 0.0, 0.0, v
    s = rangec / maxc
    if r == maxc:
        h = (g-b) / rangec
    elif g == maxc:
        h = 2.0 + (b-r) / rangec
    else:
        h = 4.0 + (r-g) / rangec
    h = (h/6.0) % 1.0
    return h, s, v

nums = [x/100 for x in range(0,100)]

def test_old():
    for r in nums:
        for g in nums:
            for b in nums:
                rgb_to_hsv_old(r,g,b)

def test_new():
    for r in nums:
        for g in nums:
            for b in nums:
                rgb_to_hsv_new(r,g,b)

print('Old:',timeit.timeit(test_old,number=10))
print('New:',timeit.timeit(test_new,number=10))

Old: 7.218379099969752
New: 6.1364087999099866

[Yay295]

stable release of 3.11.9 on Windows

[gaogaotiantian]

I can confirm a similar perf result on main. Also the hsv diff on the example is <= sys.float_info.epsilon. From my personal perspective this could be a good PR.

[gaogaotiantian]

Even though this is a perf optimization, there is user observable behavior changes - the result is a "bit" (pun intended) different. You should at least include a news entry and we can ask for @sobolevn 's opinion on the change.

[sobolevn]

@gaogaotiantian I am neutral :)

Because rgb_to_hsv is rarely a hot perf critical path in your code, I guess. But I like when the code is faster. I also don't think that the precision is critical here.

[gaogaotiantian]

I remember @terryjreedy has reviewed(and declined) one of my PRs to colorsys. Let's see if he has any objections on merging this.

[nineteendo]

Does the function become faster for greyscale values if rangec = (maxc-minc) is moved after the if check?

[Yay295]

surprisingly, no
Old: 6.133919600164518
New: 6.310812799958512

[nineteendo]

Yes:

Old: 4.684144808999999
New: 4.411844383999999

[nineteendo]

Strange, could you try running this?

import timeit

def rgb_to_hsv_old(r, g, b):
    maxc = max(r, g, b)
    minc = min(r, g, b)
    rangec = (maxc-minc)
    v = maxc
    if minc == maxc:
        return 0.0, 0.0, v
    s = rangec / maxc
    if r == maxc:
        h = (g-b) / rangec
    elif g == maxc:
        h = 2.0 + (b-r) / rangec
    else:
        h = 4.0 + (r-g) / rangec
    h = (h/6.0) % 1.0
    return h, s, v

def rgb_to_hsv_new(r, g, b):
    maxc = max(r, g, b)
    minc = min(r, g, b)
    v = maxc
    if minc == maxc:
        return 0.0, 0.0, v
    rangec = (maxc-minc)
    s = rangec / maxc
    if r == maxc:
        h = (g-b) / rangec
    elif g == maxc:
        h = 2.0 + (b-r) / rangec
    else:
        h = 4.0 + (r-g) / rangec
    h = (h/6.0) % 1.0
    return h, s, v

print('Old:',timeit.timeit(lambda: rgb_to_hsv_old(9,9,9),number=10_000_000))
print('New:',timeit.timeit(lambda: rgb_to_hsv_new(9,9,9),number=10_000_000))

[Yay295]

Old: 4.841873000143096
New: 4.7804170998279005

The arguments to rgb_to_hsv are floats in the range [0,1] though, not integers.

[nineteendo]

Yeah my bad, try with 0.5 then.

[Yay295]

Old: 4.947571000084281
New: 4.570939600002021

though with (0.2,0.5,0.7) I get

Old: 6.850879299920052
New: 6.9389673001132905

so it is faster when the numbers are the same, but slower when they aren't

[nineteendo]

No, it's just random:

Old: 5.283142290998512
New: 5.288310900999932