classification
Title: Normalize math precision in RGB/YIQ conversion
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ezio.melotti, mark.dickinson, packetslave, python-dev, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2012-03-15 17:19 by packetslave, last changed 2013-08-06 08:53 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
colorlib.patch packetslave, 2012-03-15 17:19 Old review
acks.patch packetslave, 2012-03-15 17:19 review
colorlib.patch packetslave, 2012-03-17 04:16 review
colorsys_yiq_fcc.patch serhiy.storchaka, 2013-08-04 22:23 review
colorsys_yiq_fcc_2.patch serhiy.storchaka, 2013-08-05 08:20 review
Messages (15)
msg155915 - (view) Author: Brian Landers (packetslave) Date: 2012-03-15 17:19
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.
msg156090 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-03-16 22:02
The idea seems reasonable. Do you have a link or reference to a Matlab doc with the coefficients? Enhancements only go in new versions.
msg156098 - (view) Author: Brian Landers (packetslave) Date: 2012-03-16 23:09
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?
msg156131 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-03-17 04:01
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.
msg156132 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-03-17 04:04
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?
msg156133 - (view) Author: Brian Landers (packetslave) Date: 2012-03-17 04:16
Updated to add Matlab refs, also added a roundtrip RGB-YIQ-RGB test to match the other conversions.
msg158893 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-04-20 21:24
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.
msg179323 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-08 09:16
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.
msg194433 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-04 22:23
Here is a patch which implements the FCC version of RGB/YIQ conversion.
msg194445 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-05 02:08
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.)
msg194460 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-05 08:20
> 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).
msg194481 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-08-05 14:15
LGTM.
msg194504 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-05 19:06
I agree. Go ahead and push.
msg194507 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-08-05 19:38
LGTM too.
msg194531 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-06 08:52
New changeset 80e9cb6163b4 by Serhiy Storchaka in branch 'default':
Issue #14323: Expanded the number of digits in the coefficients for the
http://hg.python.org/cpython/rev/80e9cb6163b4
History
Date User Action Args
2013-08-06 08:53:04serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2013-08-06 08:52:02python-devsetnosy: + python-dev
messages: + msg194531
2013-08-05 19:38:32mark.dickinsonsetmessages: + msg194507
2013-08-05 19:06:59terry.reedysetmessages: + msg194504
2013-08-05 14:15:46ezio.melottisetmessages: + msg194481
stage: patch review -> commit review
2013-08-05 08:20:13serhiy.storchakasetfiles: + colorsys_yiq_fcc_2.patch

messages: + msg194460
2013-08-05 02:08:23terry.reedysetmessages: + msg194445
2013-08-04 22:23:34serhiy.storchakasetfiles: + colorsys_yiq_fcc.patch

messages: + msg194433
2013-08-04 21:39:44serhiy.storchakalinkissue18563 superseder
2013-01-08 09:16:25serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg179323
2013-01-08 05:31:22ezio.melottisetnosy: + ezio.melotti
stage: patch review

versions: + Python 3.4, - Python 3.3
2012-04-20 21:24:46mark.dickinsonsetmessages: + msg158893
2012-03-17 04:16:19packetslavesetfiles: + colorlib.patch

messages: + msg156133
2012-03-17 04:04:26terry.reedysetnosy: + mark.dickinson
messages: + msg156132
2012-03-17 04:01:32terry.reedysetmessages: + msg156131
2012-03-16 23:09:49packetslavesetmessages: + msg156098
2012-03-16 22:02:07terry.reedysetnosy: + terry.reedy

messages: + msg156090
versions: - Python 2.7, Python 3.2
2012-03-15 17:19:47packetslavesetfiles: + acks.patch
2012-03-15 17:19:36packetslavecreate