[BUG]: sd_to_XYZ fail with MultiSpectralDistributions

gul916 commented 2 months ago

Description

Hello, thank you for this amazing package. It's really powerful and well-documented.

I found a bug when converting a multispectraldistribution to XYZ using 'babel_average' dataset. The expected behavior is obtained when using 'cc_ohta'. sds = colour.SDS_COLOURCHECKERS['cc_ohta']

Thank you, gul916

Code for Reproduction

import colour

# Definitions
# sds = colour.SDS_COLOURCHECKERS['cc_ohta']
sds = colour.SDS_COLOURCHECKERS['babel_average']
cmfs = colour.MSDS_CMFS['CIE 1931 2 Degree Standard Observer']
ilum_sds = colour.SDS_ILLUMINANTS['D50']

# MultiSpectralDistributions
data_sds = list(sds.values())
multi_sds = colour.colorimetry.sds_and_msds_to_msds(data_sds)

# Convert to XYZ
XYZ_sds = colour.sd_to_XYZ(multi_sds, cmfs, ilum_sds)/100

Exception Message

Traceback (most recent call last):

  File ~\anaconda3\envs\colour\Lib\site-packages\spyder_kernels\py3compat.py:356 in compat_exec
    exec(code, globals, locals)

  File ~\python\colour\untitled0.py:21
    XYZ_sds = colour.sd_to_XYZ(multi_sds)/100

  File ~\anaconda3\envs\colour\Lib\site-packages\colour\colorimetry\tristimulus_values.py:1294 in sd_to_XYZ
    XYZ = function(

  File ~\anaconda3\envs\colour\Lib\site-packages\colour\colorimetry\tristimulus_values.py:1088 in sd_to_XYZ_ASTME308
    XYZ = method(sd, cmfs, illuminant, k=k)

  File ~\anaconda3\envs\colour\Lib\site-packages\colour\colorimetry\tristimulus_values.py:884 in sd_to_XYZ_tristimulus_weighting_factors_ASTME308
    XYZ = np.sum(W * R[..., None], axis=0)

ValueError: operands could not be broadcast together with shapes (36,3) (36,24,1)

Environment Information

===============================================================================
*                                                                             *
*   Interpreter :                                                             *
*       python : 3.12.4 | packaged by Anaconda, Inc. | (main, Jun 18 2024,    *
*   15:03:56) [MSC v.1929 64 bit (AMD64)]                                     *
*                                                                             *
*   colour-science.org :                                                      *
*       colour : 0.4.4                                                        *
*                                                                             *
*   Runtime :                                                                 *
*       imageio : 2.34.2                                                      *
*       matplotlib : 3.8.4                                                    *
*       networkx : 3.3                                                        *
*       numpy : 1.26.4                                                        *
*       pandas : 2.2.2                                                        *
*       scipy : 1.13.1                                                        *
*                                                                             *
===============================================================================
Out[12]: 
defaultdict(dict,
            {'Interpreter': {'python': '3.12.4 | packaged by Anaconda, Inc. | (main, Jun 18 2024, 15:03:56) [MSC v.1929 64 bit (AMD64)]'},
             'colour-science.org': {'colour': '0.4.4'},
             'Runtime': {'imageio': '2.34.2',
              'matplotlib': '3.8.4',
              'networkx': '3.3',
              'numpy': '1.26.4',
              'pandas': '2.2.2',
              'scipy': '1.13.1'}})

KelSolaar commented 2 months ago

Hi @gul916,

colour.sd_to_XYZ does not support integration over a colour.MultiSpectralDistributions as you are doing it here. The case where a colour.MultiSpectralDistributions is supported is if it carries a single signal: https://github.com/colour-science/colour/blob/develop/colour/colorimetry/tests/test_tristimulus_values.py#L1594

Failure on our part to not make that clearer! For what you are trying to achieve here, I would recommend that you either:

Loop over every single SD and call colour.sd_to_XYZ:

XYZ = np.array([colour.sd_to_XYZ(sd, cmfs, ilum_sds) / 100 for sd in colour.SDS_COLOURCHECKERS['babel_average'].values()])

Use the integration method and process the data as if it was an array:

XYZ = colour.sd_to_XYZ(np.transpose(multi_sds.values),  cmfs, ilum_sds, method="integration", shape=colour.SpectralShape(380, 730, 10))/100

Cheers,

Thomas

gul916 commented 2 months ago

Hi @KelSolaar, Thank you for this clarification, I understand. I will modify my code acordingly. I am surprised that the same code is working with sds = colour.SDS_COLOURCHECKERS['cc_ohta']. This dataset only differs by the used spectral range and the interval of points. Can you explain the reason? Regards

KelSolaar commented 1 month ago

I will need to dig into the code to understand why it is working. I would not expect it to work but looks like it does indeed. I haven't touched this part of the codebase in a while so maybe it is somehow intentional.

I will keep you posted!

KelSolaar commented 1 month ago

Ok so I took a look, and I understand. Our Integration method is vectorised but we default to the ASTME-308 method and in the specific case of 10nm interval, it goes through the colour.colorimetry.tristimulus_weighting_factors_ASTME2022 definition which is not vectorised. For 5 nm interval it goes through colour.colorimetry.sd_to_XYZ_integration

Takeaway:

The code should support colour.MultiSpectralDistributions, I had forgotten that I implemented support for it when I did the NDArray integration support.
There is a bug and we need to handle that special case.
Fix for now would be to set method="Integration" in you colour.sd_to_XYZ call.

gul916 commented 1 month ago

Thank you @KelSolaar for investigating and for your explanation. The first method you proposed with a loop over every single SD was working well. Regards,

colour-science / colour