Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update image's luminance extraction to current recommendation. #3651

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mamrehn
Copy link
Contributor

@mamrehn mamrehn commented Jan 15, 2019

Description

[Tell us about your new feature, improvement, or fix! If relevant, please supplement the PR with images.]

The ITU-R BT.709-1 recommendation from 1993 (Y = 0.2125 R + 0.7154 G + 0.0721 B) is outdated and was replaced by BT.709-6 in 2015.
Adapt the rgb2gray conversion function accordingly to Y = 0.2126 R + 0.7152 G + 0.0722 B.

https://www.itu.int/rec/R-REC-BT.709-1-199311-S/en
https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
https://www.itu.int/rec/R-REC-BT.709/en

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

The BT.709-1 recommendation from 1993 (`Y = 0.2125 R + 0.7154 G + 0.0721  B`) is outdated and was replaced by BT.709-6 in 2015.
Adapt the rgb2gray conversion function accordingly to `Y = 0.2126 R + 0.7152 G + 0.0722 B`.

https://www.itu.int/rec/R-REC-BT.709-1-199311-S/en
https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
https://www.itu.int/rec/R-REC-BT.709/en
@pep8speaks
Copy link

pep8speaks commented Jan 15, 2019

Hello @mamrehn! Thanks for updating the PR.

Line 781:80: E501 line too long (89 > 79 characters)
Line 793:80: E501 line too long (85 > 79 characters)

Comment last updated on January 15, 2019 at 12:19 Hours UTC

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mamrehn thanks for your contribution! I've requested a small change with how you documented it, but otherwise I think it's worth getting in! Thanks!

@@ -751,7 +751,8 @@ def rgbcie2rgb(rgbcie):


def rgb2gray(rgb):
"""Compute luminance of an RGB image.
"""Compute luminance of an RGB image,
according to ITU-R BT.709-6 (2015) recommendation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this line to a Notes section (see numpy documentation guide) and add a versionchanged tag for 0.15. Here's an example. In the note, could you please note that prior to 0.15, the earlier coefficients were used? This will help future users to understand any discrepancy in their results between versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks @jni !

Help understand discrepancy in results between versions.
Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mamrehn! Looks great now!

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! I thought this might be a problem: now a whole bunch of tests are failing because we expect the old conversion. This is going to be trickier than I had hoped...

https://travis-ci.org/scikit-image/scikit-image/jobs/479854144#L2969

@mamrehn
Copy link
Contributor Author

mamrehn commented Jan 18, 2019

These 5 tests just fail due to hard coded ground truth values.
How about a new parameter like mode and rgb2gray(rgb, mode='BT.709-1') as well as rgb2gray(rgb, mode='BT.709-6') (new default) as an option?

Then modify rgb2gray(rgb) (which now uses 'BT.709-6') to rgb2gray(rgb, mode='BT.709-1') in the 5 relevant test functions?
This way, scikit-image users' test code should also be fixable with minimum effort.
Note that the old way of RGB to grayscale conversion is not wrong, just not adapted to human perception on modern (>1993) screens, so rgb2gray(rgb, mode='BT.709-1') does still make sense - at least in a test.

@hmaarrfk
Copy link
Member

I think it is wise to have different color conversion formats for rgb2gray. Though it is a 1E-3 error, and uint8 images don't even have athat precision, it might trip up some people.

Does this need deprecation? if so, it would probably only be default after it becomes obsolete again....

@sciunto sciunto added this to the 0.16 milestone Feb 21, 2019
@sciunto sciunto added the 🔧 type: Maintenance Refactoring and maintenance of internals label Feb 21, 2019
@sciunto sciunto modified the milestones: 0.16, 1.0 May 2, 2020
Base automatically changed from master to main February 18, 2021 18:23
@lagru
Copy link
Member

lagru commented Jul 30, 2022

Hey everyone. It seems like this has been stalled a little bit. I think the option to include a new mode='BT.709-1' parameter and move to mode='BT.709-6' with deprecation warnings would be resonable. However, currently it's not ensured that we will make enough releases before skimage2 to finish a full deprecation cycle (with transition to skimage2 we will be free to make breaking changes, see SKIP4 and most recent discussion #6448).

I understand that this change will lead to small changes for images >8 bit? In that case it's probably easiest to aim for a merge after that transition. So I'm updating the milestone for now. However, @mamrehn, if you want to pick this up again, we can still try to merge it before skimage2 so that the updated standard is at least available as an option.

@lagru lagru modified the milestones: 1.0, skimage2 Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚖️ Needs decision 🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants