Skip to content

PR: Datasets: TCS: Adding TCS15#1337

Merged
KelSolaar merged 2 commits intocolour-science:feature/cie2024_integrationfrom
cmuellner:tcs15
Mar 17, 2025
Merged

PR: Datasets: TCS: Adding TCS15#1337
KelSolaar merged 2 commits intocolour-science:feature/cie2024_integrationfrom
cmuellner:tcs15

Conversation

@cmuellner
Copy link
Copy Markdown
Contributor

@cmuellner cmuellner commented Mar 6, 2025 •
edited
Loading

Summary

TCS15 was added to the CIE 1995 test color samples to get a better representation of the Japanese skin complexion.

This commit adds this color sample and adjust the CRI test file accordingly.

Note, that the TCS15 dataset is published from 380-780 nm in 5 nm increments. This differs from all other TCS data, which is published from 360-830 nm (also in 5 nm increments).

Source of data is the CIE's open access dataset page: https://www.cie.co.at/datatable/spectral-radiance-factors-test-colour-sample-15-japanese-skin-complexion-5nm-wavelength

Data source reference:
CIE 2024, Spectral radiance factors of test-colour sample #15 of the Japanese skin complexion, 5nm wavelength steps, International Commission on Illumination (CIE), Vienna, AT, DOI: 10.25039/CIE.DS.7chm7z5h

Preflight

Code Style and Quality

  • Unit tests have been implemented and passed.
  • Pyright static checking has been run and passed.
  • Pre-commit hooks have been run and passed.
  • New transformations have been added to the Automatic Colour Conversion Graph.
  • New transformations have been exported to the relevant namespaces, e.g. colour, colour.models.

I observed the following fails on the develop branch (06edf82f054 with no changes on top) on my Fedora 41 (x86-64) machine:

FAILED colour/io/image.py::colour.io.image.read_image
FAILED colour/io/image.py::colour.io.image.read_image_Imageio
FAILED colour/io/tests/test_image.py::TestReadImageImageio::test_read_image_Imageio
FAILED colour/io/tests/test_image.py::TestWriteImageImageio::test_write_image_Imageio
FAILED colour/io/tests/test_image.py::TestWriteImage::test_write_image - Valu...
FAILED colour/io/tests/test_image.py::TestReadImage::test_read_image - ValueE...

This commit seems to trigger the following additional fails, but I cannot find an obvious reason for that:

FAILED colour/colorimetry/tests/test_uniformity.py::TestSpectralUniformity::test_spectral_uniformity
FAILED colour/colorimetry/uniformity.py::colour.colorimetry.uniformity.spectral_uniformity

The pre-commit hook triggers the error RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.10'.

Documentation

  • New features are documented along with examples if relevant.
  • The documentation is Sphinx and numpydoc compliant.

@cmuellner
Copy link
Copy Markdown
Contributor Author

cmuellner commented Mar 6, 2025

Force-pushed the following changes:

  • White-space issues identified by pre-commit.ci

@KelSolaar
Copy link
Copy Markdown
Member

KelSolaar commented Mar 9, 2025

Hi @cmuellner,

Thank you! The image io related tests failure is related to the fact that you probably don't have the Freeimage backend installed: uv run python -c "import imageio;imageio.plugins.freeimage.download()"

The spectral uniformity tests are on the other hand a side effect to the fact you added a new TCS sample which leads me to the following point: We should not modify the existing samples because this will change CRI and everything that depends on it. What we should probably do instead is:

  • Deprecate the colour.quality.datasets.tcs.SDS_TCS attribute and rename it to colour.quality.datasets.tcs.SDS_TCS_CIE_1995
  • Create a new colour.quality.datasets.tcs.SDS_TCS_CIE_2024 attribute with the Japanese skin sample
  • Add a new method argument to the colour.colour_rendering_index definition that takes two values, e.g., ["CIE 1995", "CIE 2024"] allowing to switch to between the two datasets. The colour.colour_quality_scale definition can be used as an example on how to proceed
  • Add unit tests for the new method

Let me know if that makes sense!

Cheers,

Thomas

@cmuellner
datasets: TCS: Adding TCS15
901a609 
TCS15 was added to the CIE 1995 test color samples to get a better
representation of the Japanese skin complexion.
This commit adds this color sample and adjust the CRI test file
to pass.

Note, that the TCS15 dataset is published from 380-780 nm in 5 nm increments.
This differs from all other TCS data, which is published from 360-830 nm
(also in 5 nm increments).

The integration of TCS15 in this commit is breaking existing code,
which can be observed in the following regressions:
* FAILED colour/colorimetry/tests/test_uniformity.py::TestSpectralUniformity::test_spectral_uniformity
* FAILED colour/colorimetry/uniformity.py::colour.colorimetry.uniformity.spectral_uniformity

The integration is addressed in a following commit.

Source of data is the CIE's open access dataset page:
https://www.cie.co.at/datatable/spectral-radiance-factors-test-colour-sample-15-japanese-skin-complexion-5nm-wavelength

Data source reference:
CIE 2024, Spectral radiance factors of test-colour sample colour-science#15 of the Japanese
skin complexion, 5nm wavelength steps, International Commission on Illumination
(CIE), Vienna, AT, DOI: 10.25039/CIE.DS.7chm7z5h

Signed-off-by: Christoph Müllner <[email protected]>
@cmuellner cmuellner force-pushed the tcs15 branch 3 times, most recently from 530b93a to 3efe76c Compare March 9, 2025 21:41
@cmuellner
Copy link
Copy Markdown
Contributor Author

cmuellner commented Mar 9, 2025

The image io related tests failure is related to the fact that you probably don't have the Freeimage backend installed: uv run python -c "import imageio;imageio.plugins.freeimage.download()"

Yes, this fixed the failing tests.

The spectral uniformity tests are on the other hand a side effect to the fact you added a new TCS sample which leads me to the following point: We should not modify the existing samples because this will change CRI and everything that depends on it. What we should probably do instead is:

  • Deprecate the colour.quality.datasets.tcs.SDS_TCS attribute and rename it to colour.quality.datasets.tcs.SDS_TCS_CIE_1995
  • Create a new colour.quality.datasets.tcs.SDS_TCS_CIE_2024 attribute with the Japanese skin sample
  • Add a new method argument to the colour.colour_rendering_index definition that takes two values, e.g., ["CIE 1995", "CIE 2024"] allowing to switch to between the two datasets. The colour.colour_quality_scale definition can be used as an example on how to proceed
  • Add unit tests for the new method

Thanks for this advice!
I've done so except for deprecating SDS_TCS (I haven't figured out how this works).

Please have a look at the changes (especially: is the naming of SDS_TCS_SETS ok for you?).

PR is rebased and all tests pass.

@cmuellner cmuellner force-pushed the tcs15 branch 2 times, most recently from 92067ee to 3f073f4 Compare March 9, 2025 21:51
@cmuellner
CRI: Fix integration of TCS15
7d40e89 
This commit fixes the TCS15 integration with the following steps:
* Adding COLOUR_RENDERING_INDEX_METHODS to switch between "CIE 1995" and
  "CIE 2024" TCS sets
* Introduce `SDS_TCS_SETS` to get the TCS set for the desired method
* Introduce SDS_TCS_CIE_1995 (was SDS_TCS before) SDS_TCS_CIE_2024
* Add new parameter `method` to colour.colour_rendering_index to select
  the TCS set
* Add a test for this new `method` parameter
* Adjust the docs to reflect these changes with inspiration from
  the symbols exported by the CQS module

Suggested-by: Thomas Mansencal <[email protected]>
Signed-off-by: Christoph Müllner <[email protected]>
@KelSolaar KelSolaar changed the title datasets: TCS: Adding TCS15 PR: Datasets: TCS: Adding TCS15 Mar 17, 2025
@KelSolaar KelSolaar changed the base branch from develop to feature/cie2024_integration March 17, 2025 06:06
@KelSolaar KelSolaar merged commit fd3d349 into colour-science:feature/cie2024_integration Mar 17, 2025
16 checks passed
@KelSolaar
Copy link
Copy Markdown
Member

KelSolaar commented Mar 17, 2025

Thanks @cmuellner! I will take it on from here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cmuellner @KelSolaar