Add new illuminants to data registry.

[mkcor]

Description

This PR is a follow-up on #5276, replacing #5234 by breaking it down into sequential PRs which pass CI.

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]

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

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

Line 311:13: E741 ambiguous variable name 'I'
Line 312:13: E741 ambiguous variable name 'I'
Line 320:13: E741 ambiguous variable name 'I'
Line 339:13: E741 ambiguous variable name 'I'
Line 340:13: E741 ambiguous variable name 'I'
Line 347:13: E741 ambiguous variable name 'I'
Line 416:13: E741 ambiguous variable name 'I'
Line 417:13: E741 ambiguous variable name 'I'
Line 425:13: E741 ambiguous variable name 'I'
Line 444:13: E741 ambiguous variable name 'I'
Line 445:13: E741 ambiguous variable name 'I'
Line 452: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 65: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 69:80: E501 line too long (109 > 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 76:80: E501 line too long (111 > 79 characters)
Line 79:80: E501 line too long (111 > 79 characters)
Line 82:80: E501 line too long (111 > 79 characters)

Comment last updated at 2021-04-19 15:27:28 UTC

[grlee77]

This looks good, thanks for helping get everything working with Pooch @mkcor. It looks like @BierretA is still be credited here as well, so that is good.

There is one other suggestion in #5223 about allowing user-supplied observer and illuminants. I think we can merge this PR as is now and close #5223, but then open a new feature request issue just for that idea so it doesn't get lost.

[mkcor]

This looks good, thanks for helping get everything working with Pooch @mkcor.

Sure, then I need to document it (#5277).

It looks like @BierretA is still be credited here as well, so that is good.

Yes! I cherry-picked what I could from #5234 and also used Git's Co-authored-by feature. Sorry, I hadn't ported everything from #5234 when I opened this PR; thanks for reviewing so promptly, @grlee77! It should be good now.

I don't know whether @BierretA would endorse my choice, but I didn't want duplicate files in our data registry: Actually, I caught it for luv_array_... but not for lab_array_... (noticing right now as I'm typing), but basically the [illuminant, observer] combination [D75, R] is the same as [D75, 2], while the [E, 10] and [E, R] combinations are the same as [E, 2].

There is one other suggestion in #5223 about allowing user-supplied observer and illuminants. I think we can merge this PR as is now and close #5223, but then open a new feature request issue just for that idea so it doesn't get lost.

Thanks for pointing this out, I didn't have it in mind. Sounds good!

[BierretA]

Hello @mkcor ,
Sorry I could not answer faster, I do not have access to my usual computer since last week.

I don't know whether @BierretA would endorse my choice, but I didn't want duplicate files in our data registry: Actually, I caught it for luv_array_... but not for lab_array_... (noticing right now as I'm typing), but basically the [illuminant, observer] combination [D75, R] is the same as [D75, 2], while the [E, 10] and [E, R] combinations are the same as [E, 2].

I completely agree with your your modifications, avoiding duplicate files is better. What you did for luv array tests is good, and the same should be done for lab array tests.

[mkcor]

Hi @BierretA!

I completely agree with your modifications, avoiding duplicate files is better. What you did for luv array tests is good, and the same should be done for lab array tests.

Very good, so I'll update the lab data files to avoid duplicates, and the data registry and lab tests accordingly.

Still, does it make sense to have built-in duplicates, I mean in the illuminants' definition? It looks like we should edit skimage/color/colorconv.py as well. I would need to dig into the R docs and references therein to double-check, but I would be surprised to find out that, say, [D75, R] would be the same as [D75, 2] by definition. Would you happen to know, off the top of your head, where we can find the info? Thank you!

[mkcor]

Ok, illuminant E gives equal weight to all wavelengths by definition, so its tristimulus values (X, Y, Z) are (1.0, 1.0, 1.0) regardless of the observer's field of view.

I haven't found a reference for it, but I imagine that, for illuminant D75, the R observer happens to be the standard observer.

[mkcor]

Ok, now I'm slightly more familiar with the world of illuminants, I'm at peace with this PR ☺️

[BierretA]

Still, does it make sense to have built-in duplicates, I mean in the illuminants' definition? It looks like we should edit skimage/color/colorconv.py as well. I would need to dig into the R docs and references therein to double-check, but I would be surprised to find out that, say, [D75, R] would be the same as [D75, 2] by definition. Would you happen to know, off the top of your head, where we can find the info? Thank you!

Illuminants A, B, C and E in R were indeed defined to be the same as the illuminants with a 2° observer. Unfortunately, the information is not clear in the R-documentation can only be found in the source code (it used to be accessible from https://www.rdocumentation.org/packages/grDevices/versions/3.6.2, but it looks like it was recently changed?).

The value for illuminant D75 is missing in R. I added it here to keep the keys as '2', '10', and 'R' for every illuminant (I chose the same value as the 2° observer, like illuminants A/B/C/E). If having different keys only for D75 is not a problem, this should be removed.

[mkcor]

Thank you, @BierretA! I sort of wrapped my head around these illuminants and I'm enjoying the learning process.

The value for illuminant D75 is missing in R. I added it here to keep the keys as '2', '10', and 'R' for every illuminant (I chose the same value as the 2° observer, like illuminants A/B/C/E). If having different keys only for D75 is not a problem, this should be removed.

Either approach makes sense, but I like it this way. It's a 'natural' extension, I would say.

Illuminants A, B, C and E in R were indeed defined to be the same as the illuminants with a 2° observer.

Ok, good to know: They are defined to be the same in terms of tristimulus values (X, Y, Z), as we can see in skimage/color/colorconv.py. But then... shouldn't file "color/tests/data/lab_array_a_r.npy" end up being the same as "color/tests/data/lab_array_a_2.npy" (and so on)?

Now, I have to look into the failing CI tests... 😅

[BierretA]

Ok, good to know: They are defined to be the same in terms of tristimulus values (X, Y, Z), as we can see in skimage/color/colorconv.py. But then... shouldn't file "color/tests/data/lab_array_a_r.npy" end up being the same as "color/tests/data/lab_array_a_2.npy" (and so on)?

Yes. And actually, we can remove them (for illuminants A, B and C) and update the tests accordingly. I made the modifications on my fork but I cannot push it directly here?

[mkcor]

Hi @BierretA,

Thank you so much for your responsiveness!

I made the modifications on my fork but I cannot push it directly here?

That's because 'here' is my branch, so it's expected that you wouldn't have write (push) permission on it.

I would suggest that we @grlee77 @hmaarrfk merge this current PR first; even if further cleanup can be made, it makes sense and it's formally consistent as is. Then you @BierretA can submit a PR with the additional cleanup.

Yes. And actually, we can remove them (for illuminants A, B and C) and update the tests accordingly.

Ok, the illuminants are the same by definition, but "color/tests/data/lab_array_a_r.npy", i.e,

array([[[  53.23288,   62.2834 ,   43.65972]],

       [[   0.     ,    0.     ,    0.     ]],

       [[ 100.     ,  -23.54141,  -90.37643]],

       [[  32.30259,   65.6623 , -194.2238 ]],

       [[  46.22836,  -61.89611,   23.96719]]])

is 'different' from "color/tests/data/lab_array_a_2.npy", i.e.,

array([[[  53.23288179,   62.27968785,   43.66165206]],

       [[   0.        ,    0.        ,    0.        ]],

       [[ 100.        ,  -23.54631417,  -90.36903339]],

       [[  32.30258667,   65.65947843, -194.21673374]],

       [[  46.22835703,  -61.89823312,   23.96931025]]])

because it uses fewer decimals. In your cleanup PR, would you be able to replace the new R illuminants with their double-precision version? This could also be a third, separate PR, no stress.

PS: Failing CI tests were unrelated and have been fixed in the meantime.

[BierretA]

In your cleanup PR, would you be able to replace the new R illuminants with their double-precision version? This could also be a third, separate PR, no stress.

Sure. I will wait for this PR to be merged and make a new one.

[alexdesiqueira]

Thank you @BierretA @mkcor!