Incorrect values returned by Color.hsv()

[rrjjvv]

A longer writeup is located at http://help.simplecv.org/question/1722/colorhsv-clarification-possible-bug/ but was unsure if my findings were a bug, but a commenter referred me to here, implying it was. Important details are copied here.

Problem

I believe Color.hsv() is returning incorrect values due to misuse of colorsys.rgb_to_hsv, which requires parameters in a 0-1 range, but is handed 'standard' RGB values (0-255), which results in incorrect results.

Details

In Color.py, the source for Color.hsv() reads:

def hsv(cls, tuple):
    hsv_float = colorsys.rgb_to_hsv(*tuple)
    return (hsv_float[0] * 180, hsv_float[1] * 255, hsv_float[2])

The documentation for colorsys reads:

All inputs and outputs are triples of floats in the range [0.0...1.0]
(with the exception of I and Q, which covers a slightly larger range).
Inputs outside the valid range may cause exceptions or invalid outputs.

In the docstring for Color.hsv(), an example usage (showing that the method wants an integral RGB vs. a scaled version) is:

    >>> c = Color.RED
    >>> hsvc = Color.hsv(c)

Below is a small script detailing how the existing test in the test suite passes for one color (which, incidentally, is wrong... Color.GREEN should be (0, 255, 0) and not (0, 128,0)), but fails for 20 of 36 of Color.colorlist colors. Included is a possible implementation that allows the test to pass for all Color.colorlist colors.

Also, note that while the Color.hsv() method is only called in a few places, there are multiple places where colorsys methods are called with unscaled RGB values. If the sample implementation below looks good, I'd be happy to make a proper patch to attempt fixing all of these issues.

Potential fix

#!/usr/bin/python

import colorsys

from SimpleCV import Color
from SimpleCV.tests import tests

DO_ALL = True

TRUE_GREEN = {'name': 'true green',
              'color': (0, 255, 0)}
STOCK_GREEN = {'name': 'stock green',
               'color': Color.GREEN}
MY_COLOR = {'name': 'problematic color',
                'color': (0, 45, 150)}

def color_test(colorinfo):
    Color.GREEN = colorinfo['color']
    print 'Running test with {0} ({1})'.format( colorinfo['name'],
                                                colorinfo['color'] )
    ret = 0
    try:
        tests.test_hsv_conversion()
    except AssertionError:
        print '  Failed!', Color.hsv(Color.GREEN)
        ret += 1
    return ret

def hsv(cls, tup):
    # This line is the most important    
    float_tup = tuple([i/255.0 for i in tup])

    # No change
    hsv_float = colorsys.rgb_to_hsv(*float_tup)

    # Only change is multiplying value by 255 and rounding...
    # the rounding is only to make the tests pass.  I don't know
    # whether the round() is correct or whether the tests should
    # be adapted to comparing floating point values
    return ( round(hsv_float[0] * 180),
             round(hsv_float[1] * 255),
             round(hsv_float[2] * 255) )

if __name__ == '__main__':
    if Color.GREEN != TRUE_GREEN['color']:
        print 'Stock green ({0}) is not true green ({1})'.format( Color.GREEN,
                                                                  TRUE_GREEN['color'] )
    failed = 0
    print '=' * 10, 'Regular Color.hsv', '=' * 10
    failed += color_test(STOCK_GREEN)
    failed += color_test(TRUE_GREEN)
    failed += color_test(MY_COLOR)
    if DO_ALL:
        for color in Color.colorlist:
            failed += color_test( {'color': color,
                                   'name': 'random color'} )
    print '=' * 10, 'Failed', failed, 'times', '=' * 10

    prev = Color.hsv
    Color.hsv = classmethod(hsv)

    failed = 0
    print '=' * 10, 'New Color.hsv', '=' * 10
    failed += color_test(STOCK_GREEN)
    failed += color_test(TRUE_GREEN)
    failed += color_test(MY_COLOR)
    if DO_ALL:
        for color in Color.colorlist:
            failed += color_test( {'color': color,
                                   'name': 'random color'} )
    print '=' * 10, 'Failed', failed, 'times', '=' * 10