Add new illuminants #5234

BierretA · 2021-02-12T13:32:51Z

Description

This small PR adds new white points for color conversions. See Issue #5223 .

I added illuminants B and C, as well as a new observer compatible with color conversions done with the R language.

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

For reviewers

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.

pep8speaks · 2021-02-12T13:32:56Z

Hello @BierretA! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file skimage/color/colorconv.py:

Line 421:80: E501 line too long (83 > 79 characters)

In the file skimage/color/tests/test_colorconv.py:

Line 313:13: E741 ambiguous variable name 'I'
Line 333:13: E741 ambiguous variable name 'I'
Line 405:13: E741 ambiguous variable name 'I'
Line 425:13: E741 ambiguous variable name 'I'

In the file skimage/data/_registry.py:

Line 44:80: E501 line too long (110 > 79 characters)
Line 46:80: E501 line too long (109 > 79 characters)
Line 47:80: E501 line too long (110 > 79 characters)
Line 48:80: E501 line too long (109 > 79 characters)
Line 49:80: E501 line too long (109 > 79 characters)
Line 50:80: E501 line too long (110 > 79 characters)
Line 51:80: E501 line too long (109 > 79 characters)
Line 52:80: E501 line too long (109 > 79 characters)
Line 55:80: E501 line too long (111 > 79 characters)
Line 58:80: E501 line too long (111 > 79 characters)
Line 61:80: E501 line too long (111 > 79 characters)
Line 64:80: E501 line too long (111 > 79 characters)
Line 66:80: E501 line too long (110 > 79 characters)
Line 67:80: E501 line too long (109 > 79 characters)
Line 68:80: E501 line too long (110 > 79 characters)
Line 70:80: E501 line too long (109 > 79 characters)
Line 71:80: E501 line too long (110 > 79 characters)
Line 72:80: E501 line too long (109 > 79 characters)
Line 73:80: E501 line too long (109 > 79 characters)
Line 74:80: E501 line too long (110 > 79 characters)
Line 75:80: E501 line too long (109 > 79 characters)
Line 76:80: E501 line too long (109 > 79 characters)
Line 79:80: E501 line too long (111 > 79 characters)
Line 82:80: E501 line too long (111 > 79 characters)
Line 85:80: E501 line too long (111 > 79 characters)
Line 88:80: E501 line too long (111 > 79 characters)
Line 89:80: E501 line too long (110 > 79 characters)
Line 91:80: E501 line too long (109 > 79 characters)

mkcor

Exciting addition, thanks!

mkcor · 2021-02-16T20:17:34Z

skimage/color/colorconv.py

        The name of the illuminant (the function is NOT case sensitive).
-    observer : {"2", "10"}, optional
+    observer : {"2", "10", "R"}, optional
        The aperture angle of the observer.


Suggested change

The aperture angle of the observer.

One of: 2-degree observer, 10-degree observer, or 'R' observer as in

R function grDevices::convertColor.

mkcor · 2021-02-16T20:18:24Z

skimage/color/colorconv.py

        The name of the illuminant (the function is NOT case sensitive).
-    observer : {"2", "10"}, optional
+    observer : {"2", "10", "R"}, optional
        The aperture angle of the observer.


Update here as well, etc.

BierretA · 2021-02-17T15:49:43Z

Unit tests work on my system but not with github actions.
It looks likes some tests fail because Pooch cannot fetch the new data files. I do not know why though.

mkcor · 2021-02-25T19:57:13Z

Hello @BierretA,

It definitely looks like an issue with Pooch via GitHub Actions (or vice versa). Tests run fine locally for me as well. Looking into the first failure (TestColorconv.test_lab2xyz), we can see that, over GitHub Actions, Pooch is attempting the following

Downloading file 'color/tests/data/lab_array_a_10.npy' from 'https://github.com/scikit-image/scikit-image/raw/main/skimage/color/tests/data/lab_array_a_10.npy' to '/home/runner/.cache/scikit-image/main'.

Unsurprisingly, the expected file doesn't live at https://github.com/scikit-image/scikit-image/raw/main/skimage/color/tests/data/ (yet) since skimage/color/tests/data/lab_array_a_10.npy ships with this PR!

I know I didn't run into any issue with Pooch when I added data files (#4939 merged on Sep 2, 2020). Actually, it is the first time (with this PR) we've added files in the data registry since we started running tests with GitHub Actions. Indeed, the time sequence goes like this:

Oct 31, 2020: merge latest additions to data registry (Add PIV challenge images to our data #5041)
Nov 26, 2020: start running test suite with GitHub Actions (github actions for tests #5074)

I need to dig deeper to find out how Pooch builds paths, unless someone else at @scikit-image/core knows it right away. Thanks!

stefanv · 2021-02-26T08:43:50Z

@mkcor This change was recently made—I hope that's not the culprit.

mkcor · 2021-02-26T11:48:32Z

@mkcor This change was recently made—I hope that's not the culprit.

@stefanv I know, initially I thought about that, but apparently it's a different issue: CI tests initially ran 2 weeks ago, before this change was made, and failed in the same way. Thanks though.

mkcor · 2021-03-18T11:12:33Z

skimage/data/_registry.py

    "color/tests/data/luv_array_d75_10.npy": "e1cc70d56eb6789633d4c2a4059b9533f616a7c8592c9bd342403e41d72f45e4",
    "color/tests/data/luv_array_d75_2.npy": "07db3bd59bd89de8e5ff62dad786fe5f4b299133495ba9bea30495b375133a98",
+    "color/tests/data/luv_array_d75_r.npy": "07db3bd59bd89de8e5ff62dad786fe5f4b299133495ba9bea30495b375133a98",


hash must be wrong since it's the same as line above

oh ok, files are the same

mkcor · 2021-03-18T11:12:52Z

skimage/data/_registry.py

    "color/tests/data/luv_array_d75_10.npy": "e1cc70d56eb6789633d4c2a4059b9533f616a7c8592c9bd342403e41d72f45e4",
    "color/tests/data/luv_array_d75_2.npy": "07db3bd59bd89de8e5ff62dad786fe5f4b299133495ba9bea30495b375133a98",
+    "color/tests/data/luv_array_d75_r.npy": "07db3bd59bd89de8e5ff62dad786fe5f4b299133495ba9bea30495b375133a98",
+    "color/tests/data/luv_array_e_10.npy": "41b1037d81b267305ffe9e8e97e0affa9fa54b18e60413b01b8f11861cb32213",


hash must be wrong since it's the same as below

oh ok, files are the same

mkcor · 2021-03-18T11:13:09Z

skimage/data/_registry.py

    "color/tests/data/luv_array_e_2.npy": "41b1037d81b267305ffe9e8e97e0affa9fa54b18e60413b01b8f11861cb32213",
+    "color/tests/data/luv_array_e_r.npy": "41b1037d81b267305ffe9e8e97e0affa9fa54b18e60413b01b8f11861cb32213",


hash must be wrong since it's the same as above

oh ok, files are the same

hmaarrfk

This PR looks solid.

The only thing missing is a release note, but we don't have a good place to put them.

hmaarrfk · 2021-03-22T03:44:37Z

i'm not too worried about the failing tests.

The data is in the right place, and the registry looks great. Thanks for the addition! and the thoroughness!

mkcor · 2021-03-22T09:28:51Z

@hmaarrfk this PR needs a little clean-up before merging (wrt lab_B_2.npy , luv_array_d75_r, luv_array_e_r.npy luv_array_e_10.npy as I pointed out, see also my code review comments). But I agree that, conceptually, it would be fine to merge since tests would pass on main.

I think we can have a clean, linear (sequential) commit history by re-doing the addition of 'new illuminants' from a clean slate, which I have started with #5276. Then, we can document the process as @stefanv suggested, so that each PR (submitted in the proper order) can pass all CI tests.

hmaarrfk · 2021-03-22T11:18:12Z

I think the commit history cleanup is superfluous. We can squash and merge on our end. No need to push that effort onto @BierretA .

The other cleanup is all worthwhile

mkcor · 2021-03-22T11:24:00Z

No need to push that effort onto @BierretA

@hmaarrfk totally!! Couldn't agree more, that's why I offered to take over at #5276 -- I just forgot to declare @BierretA as a co-author when committing, sorry about that. Will do in subsequent, related commits.

alexdesiqueira · 2021-04-26T22:25:06Z

Assuming that #5308 continues the work started here, and that one was merged, I am closing this one. Thank you @mkcor @BierretA @hmaarrfk for your work!

stefanv · 2021-04-26T22:27:40Z

Thank you @BierretA!

BierretA added 11 commits February 5, 2021 16:17

Add new illuminants in colorconv.py

4159e0f

Change docstring and modifiy tests

0b5a6d1

Modify xyz-lab color conversion and add new test matrices

c7b9935

Update registry of datafiles w/ lab_array hashes

3a4b621

Format observer in get_xyz_coords

7967237

Fix npy array format

23178fb

Update Luv-XYZ conversion tests

9664033

Fix Luv A 10 test matrix

fbca5eb

Fix Luv A R matrix format

1f8b0d4

Fix Luv C and B matrix values

825edf3

Update docstrings

e834e85

grlee77 mentioned this pull request Feb 15, 2021

2021's calendar of community management #5169

Closed

mkcor reviewed Feb 16, 2021

View reviewed changes

Base automatically changed from master to main February 18, 2021 18:23

mkcor mentioned this pull request Mar 10, 2021

Pooch paths don't work with GitHub Actions. #5267

Closed

grlee77 added the ⏩ type: Enhancement Improve existing features label Mar 11, 2021

mkcor reviewed Mar 18, 2021

View reviewed changes

mkcor mentioned this pull request Mar 18, 2021

Add data files for new illuminants. #5276

Merged

hmaarrfk approved these changes Mar 22, 2021

View reviewed changes

mkcor mentioned this pull request Mar 31, 2021

Add new illuminants to data registry. #5308

Merged

alexdesiqueira closed this Apr 26, 2021

mkcor mentioned this pull request Jun 7, 2021

Added ND butterworth filter #5382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new illuminants #5234

Add new illuminants #5234

BierretA commented Feb 12, 2021

pep8speaks commented Feb 12, 2021

mkcor left a comment

mkcor Feb 16, 2021

mkcor Feb 16, 2021

BierretA commented Feb 17, 2021

mkcor commented Feb 25, 2021

stefanv commented Feb 26, 2021

mkcor commented Feb 26, 2021

mkcor Mar 18, 2021

mkcor Mar 18, 2021

mkcor Mar 18, 2021

mkcor Mar 18, 2021

mkcor Mar 18, 2021

mkcor Mar 18, 2021

hmaarrfk left a comment

hmaarrfk commented Mar 22, 2021

mkcor commented Mar 22, 2021

hmaarrfk commented Mar 22, 2021

mkcor commented Mar 22, 2021

alexdesiqueira commented Apr 26, 2021

stefanv commented Apr 26, 2021

	The aperture angle of the observer.
	One of: 2-degree observer, 10-degree observer, or 'R' observer as in
	R function grDevices::convertColor.

		"color/tests/data/luv_array_e_2.npy": "41b1037d81b267305ffe9e8e97e0affa9fa54b18e60413b01b8f11861cb32213",
		"color/tests/data/luv_array_e_r.npy": "41b1037d81b267305ffe9e8e97e0affa9fa54b18e60413b01b8f11861cb32213",

Add new illuminants #5234

Add new illuminants #5234

Conversation

BierretA commented Feb 12, 2021

Description

Checklist

For reviewers

pep8speaks commented Feb 12, 2021

mkcor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BierretA commented Feb 17, 2021

mkcor commented Feb 25, 2021

stefanv commented Feb 26, 2021

mkcor commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmaarrfk left a comment

Choose a reason for hiding this comment

hmaarrfk commented Mar 22, 2021

mkcor commented Mar 22, 2021

hmaarrfk commented Mar 22, 2021

mkcor commented Mar 22, 2021

alexdesiqueira commented Apr 26, 2021

stefanv commented Apr 26, 2021