Normalize math precision in RGB/YIQ conversion

[packetslave]

BPO	14323
Nosy	@terryjreedy, @mdickinson, @ezio-melotti, @serhiy-storchaka
Files	colorlib.patch: Old acks.patch colorlib.patch colorsys_yiq_fcc.patch colorsys_yiq_fcc_2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-08-06.08:53:04.530>
created_at = <Date 2012-03-15.17:19:36.927>
labels = ['type-feature', 'library']
title = 'Normalize math precision in RGB/YIQ conversion'
updated_at = <Date 2013-08-06.08:53:04.529>
user = 'https://bugs.python.org/packetslave'

bugs.python.org fields:

activity = <Date 2013-08-06.08:53:04.529>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2013-08-06.08:53:04.530>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2012-03-15.17:19:36.927>
creator = 'packetslave'
dependencies = []
files = ['24866', '24867', '24895', '31161', '31163']
hgrepos = []
issue_num = 14323
keywords = ['patch']
message_count = 15.0
messages = ['155915', '156090', '156098', '156131', '156132', '156133', '158893', '179323', '194433', '194445', '194460', '194481', '194504', '194507', '194531']
nosy_count = 6.0
nosy_names = ['terry.reedy', 'mark.dickinson', 'ezio.melotti', 'python-dev', 'serhiy.storchaka', 'packetslave']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14323'
versions = ['Python 3.4']

[packetslave]

There doesn't seem to be a standard definition for the constants to use when doing the matrix calculations to convert RGB to YIQ or vise versa.

Also, the current colorsys library uses two digits of precision for RGB-YIQ but six digits for YIQ-RGB.

The attached patch standardizes both functions to use the same constants as Matlab, using the same 3 digits of precision. This makes a roundtrip of RGB->YIQ->RGB return the original values (for 3 digits of precision).

Also added tests, which brings colorsys.py to 100% coverage.

[terryjreedy]

The idea seems reasonable. Do you have a link or reference to a Matlab doc with the coefficients? Enhancements only go in new versions.

[packetslave]

Matlab docs are here:

• http://www.mathworks.com/help/toolbox/images/ref/rgb2ntsc.html
• http://www.mathworks.com/help/toolbox/images/ref/ntsc2rgb.html

Should these be referenced in the source itself?

re new versions: sure, I'll create a separate patch for adding the tests to the maintenance releases so there's coverage there, too. Should that have a new bug as well?

[terryjreedy]

Yes, put reference in source. I checked that you copied correctly.
I think there is some controversy, which I do not understand, about adding tests to maintenance releases without a bug fix. I will ask, so do not do it yet.

[terryjreedy]

Mark, I know you have worked on numerical algorithms.

1. Do you agree that this is a reasonable change?
2. Should new tests go in maintenance release?

[packetslave]

Updated to add Matlab refs, also added a roundtrip RGB-YIQ-RGB test to match the other conversions.

[mdickinson]

Terry: sorry, I missed this before.

Re 1. Sure, this seems reasonable, if there's a real sense in which the new numbers are better than the old. Besides MATLAB, there's also a set of numbers given on Wikipedia that might be considered. I don't have the domain knowledge to know what's sensible here.

I *would* rather see the inverse transformation keep the full 6 digits of precision, though. Or perhaps even give the inverse to full float precision. Without that, the result of roundtripping RGB -> YIQ -> RGB could be significantly (perhaps even visibly) different from the original. I don't think it's acceptable for the roundtrip error to increase significantly w.r.t. Python 3.2.

Re 2. For this issue, I don't see a real benefit to adding the tests to the maintenance releases. No strong opinions, though.

[serhiy-storchaka]

According to Wikipedia FCC conversion is defined as:

    y = 0.30*r + 0.59*g + 0.11*b
    i = 0.5990*r - 0.2773*g - 0.3217*b
    q = 0.2130*r - 0.5251*g + 0.3121*b

and non-FCC conversion is defined as:

    y = 0.299*r + 0.587*g + 0.114*b
    i = 0.595716*r - 0.274453*g - 0.321263*b
    q = 0.211456*r - 0.522591*g + 0.311135*b

Our current code

    y = 0.30*r + 0.59*g + 0.11*b
    i = 0.60*r - 0.28*g - 0.32*b
    q = 0.21*r - 0.52*g + 0.31*b

looks like FCC conversion with the precision of two decimal places. Actually with this precision the difference between the different conversions are almost absent.

[serhiy-storchaka]

Here is a patch which implements the FCC version of RGB/YIQ conversion.

[terryjreedy]

Can you add a reference for the coefficients?

I believe

def test_main():
 test.support.run_unittest(ColorsysTest)

if __name__ == "__main__":
 test_main

has been and is being replaced in other test files with

if __name__ == "__main__":
 unittest.main

and should be here.

This should get a short What's New entry in the library section, something like

colorsys:
"The number of digits in the coefficients for the RGB -- YIQ conversions have been expanded so that they match the FCC NTSC versions. The change in results should be less than 1% and may better match results found elsewhere."

(You claim about the current rounding is not exactly correct. While .28*g rounds .277 rather than .274, the current .52*g rounds the non-FCC .523 rather than the FCC .5251. So I avoided making the claim in the suggested entry. It is not important.)

[serhiy-storchaka]

Can you add a reference for the coefficients?

I have only link to Wikipedia which refers to Code of Federal Regulations §73.682. This link (http://en.wikipedia.org/wiki/YIQ) already mentioned at the top of the file.

(You claim about the current rounding is not exactly correct. While .28*g rounds .277 rather than .274, the current .52*g rounds the non-FCC .523 rather than the FCC .5251. So I avoided making the claim in the suggested entry. It is not important.)

A sum of coefficients in this line should be 0 (Q=0 for R=G=B).

Patch updated. I added a What's New entry and update to use of unittest.main(), rewrite rgb_to_yiq() in the form as in Wikipedia (it uses less multiplications) and write coefficients in yiq_to_rgb() with maximal precision (as calculated with Python).

[ezio-melotti]

LGTM.

[terryjreedy]

I agree. Go ahead and push.

[mdickinson]

LGTM too.

[python-dev]

New changeset 80e9cb6163b4 by Serhiy Storchaka in branch 'default':
Issue bpo-14323: Expanded the number of digits in the coefficients for the
http://hg.python.org/cpython/rev/80e9cb6163b4