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

PR: Implement support for "MacAdam Limits" computation. #768

Open
wants to merge 47 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
16ae787
Feature MacAdam_Limits
gutenzwerg Feb 1, 2021
47883a8
Feature MacAdam_Limits
gutenzwerg Feb 1, 2021
9e27d71
Small change in the comments
gutenzwerg Feb 20, 2021
f204568
Feature MacAdam_Limits
gutenzwerg Feb 1, 2021
99872c3
Update macadam_limits.py
gutenzwerg Feb 17, 2021
308712e
Update macadam_limits.py
gutenzwerg Feb 17, 2021
6d34507
Adjust spelling
zachlewis Feb 18, 2021
5b8593a
Update colour/volume/macadam_limits
gutenzwerg Feb 18, 2021
afc9946
Update to get results between 0 and 1
gutenzwerg Feb 19, 2021
170eb03
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 23, 2022
5cbd91d
Update macadam_limits.py
gutenzwerg May 23, 2022
d6fe7ee
Update macadam_limits.py
gutenzwerg May 23, 2022
b03eaf6
Trigger ci
tigerxy May 23, 2022
f3bc4fb
Update macadam_limits.py
gutenzwerg Jun 16, 2022
7296c92
Merge branch 'colour-science:develop' into feature/macadam_limits
gutenzwerg Jun 16, 2022
f5ee2a4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 16, 2022
388c0f6
Update macadam_limits.py
gutenzwerg Jun 16, 2022
91c45f7
Update macadam_limits.py
gutenzwerg Jun 16, 2022
6eb76d5
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 16, 2022
41a277b
Update macadam_limits.py
gutenzwerg Jun 16, 2022
977be68
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 16, 2022
9dc7145
Update macadam_limits.py
gutenzwerg Jun 16, 2022
6f306dc
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 16, 2022
ac12a6b
Update macadam_limits.py
gutenzwerg Jun 16, 2022
75ade26
Update macadam_limits.py
gutenzwerg Jun 16, 2022
463a1b2
Update macadam_limits.py
gutenzwerg Jun 16, 2022
92a1cd1
Trigger CI
tigerxy Jun 16, 2022
78141ce
Update macadam_limits.py
gutenzwerg Jun 17, 2022
a190c00
Update macadam_limits.py
gutenzwerg Jun 17, 2022
c67984d
Update macadam_limits.py
gutenzwerg Jun 17, 2022
a603c18
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 17, 2022
0659c32
Update macadam_limits.py
gutenzwerg Jun 17, 2022
f91801f
Update macadam_limits.py
gutenzwerg Jun 17, 2022
95a446e
Update macadam_limits.py
gutenzwerg Jun 21, 2022
38b727a
Update macadam_limits.py
gutenzwerg Jun 21, 2022
fa6485b
Update macadam_limits.py
gutenzwerg Jun 21, 2022
c1cd25a
Update macadam_limits.py
gutenzwerg Jun 21, 2022
9f28353
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 21, 2022
e58c32b
Update macadam_limits.py
gutenzwerg Jun 22, 2022
fd3a52f
Update macadam_limits.py
gutenzwerg Jul 7, 2022
5d817da
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 7, 2022
c87523b
Update macadam_limits.py
gutenzwerg Jul 7, 2022
31f2dbd
Update macadam_limits.py
gutenzwerg Jul 7, 2022
5ed84a0
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 7, 2022
cbdb8b4
Update macadam_limits.py
gutenzwerg Jul 7, 2022
d1eca3b
Update macadam_limits.py
gutenzwerg Jul 7, 2022
b5f06c8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 7, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions colour/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,7 @@
is_within_mesh_volume,
is_within_pointer_gamut,
is_within_visible_spectrum,
macadam_limits,
)
from .graph import describe_conversion_path, convert

Expand Down Expand Up @@ -847,6 +848,7 @@ def __getattr__(self, attribute) -> Any:
"RGB_colourspace_volume_MonteCarlo",
"RGB_colourspace_volume_coverage_MonteCarlo",
"is_within_macadam_limits",
"macadam_limits",
"is_within_mesh_volume",
"is_within_pointer_gamut",
"is_within_visible_spectrum",
Expand Down
2 changes: 2 additions & 0 deletions colour/volume/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from .datasets import * # noqa
from . import datasets
from .macadam_limits import is_within_macadam_limits
from .macadam_limits import macadam_limits
from .mesh import is_within_mesh_volume
from .pointer_gamut import is_within_pointer_gamut
from .spectrum import (
Expand All @@ -22,6 +23,7 @@
__all__ += [
"is_within_macadam_limits",
]
__all__ += ["macadam_limits"]
__all__ += [
"is_within_mesh_volume",
]
Expand Down
197 changes: 196 additions & 1 deletion colour/volume/macadam_limits.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@
from colour.models import xyY_to_XYZ
from colour.volume import OPTIMAL_COLOUR_STIMULI_ILLUMINANTS
from colour.utilities import CACHE_REGISTRY, validate_method
from colour.colorimetry import MSDS_CMFS, SpectralShape, SDS_ILLUMINANTS

__author__ = "Colour Developers"
__author__ = "Colour Developers", "Christian Greim"
__copyright__ = "Copyright 2013 Colour Developers"
__license__ = "New BSD License - https://opensource.org/licenses/BSD-3-Clause"
__maintainer__ = "Colour Developers"
Expand Down Expand Up @@ -142,3 +143,197 @@ def is_within_macadam_limits(
simplex = np.where(simplex >= 0, True, False)

return simplex


def macadam_limits(target_brightness, illuminant=()):
Copy link
Member

Choose a reason for hiding this comment

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

target_brightness: Is it really brightness here or luminance?

The floating-point type in the docstrings hints at the fact that the code is not vectorised and thus a user would need to loop to get the limits for many targets.

We should also have typing annotations here.

illuminant=(): This should be better expressed at illuminant=None and if it is not passed, we default to E.

We should also pass the CMFS.

The return value should also be annotated.

The signature should probably be be something along those lines:

macadam_limits(
    luminance: FloatingOrNDArray,
    cmfs: Optional[MultiSpectralDistributions] = None,
    illuminant: Optional[SpectralDistribution] = None,
) -> NDarray:

Copy link
Author

@gutenzwerg gutenzwerg Jun 22, 2022

Choose a reason for hiding this comment

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

Notation with ":" raises errors in "Static Type Checking / macOS-latest - Python 3.9 (pull_request)", only in this test. I did it (I fear not to Your satisfaction) by using normal default-definitions:
def macadam_limits(
luminance: Floating = 0.5,
illuminant = SDS_ILLUMINANTS["E"],
spectral_range = SpectralShape(360, 830, 1),
cmfs = MSDS_CMFS["CIE 1931 2 Degree Standard Observer"],
) -> NDArray:

Is there perhaps a complete description of the colour.hints? I simply did not find out how to describe SpectralShape, Illuminant and cmfs analog to "Floating". Attempts like:
illuminant: Optional[SpectralDistribution] = SDS_ILLUMINANTS["E"]
throw errors as descibed above. I assume a systematic problem in Static Type Checking.

Copy link
Member

Choose a reason for hiding this comment

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

This initial docstring is just too long. We should also not constrain the user to a particular spectral domain, the definition should handle that for him according to the passed CMFS.

Copy link
Author

Choose a reason for hiding this comment

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

I do not exactly understand what ist ment, but I did a more sophisticated solution in the actual code.

Copy link
Member

Choose a reason for hiding this comment

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

This does not really explain what target_brightness.

Copy link
Author

Choose a reason for hiding this comment

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

target_brightness is replaced by luminance in actual code

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

As I said above, the solutions at the link with definitions of ":" are raising errors in "Static Type Checking / macOS-latest - Python 3.9 (pull_request)", only in this particular test.

"""
Wavelength reaches from 360 to 830 nm, in within the program it is
handled as 0 to 470. Beyond the references this program is very fast,
because the possible optimums are not simply tested step by step but
more effectively targeted by steps of power of two. The wavelengths
left and right of a rough optimum are fitted by a rule of proportion,
so that the wished brightness will be reached exactly.

Parameters
----------
target_brightness : floating point
brightness has to be between 0 and 1

illuminant: object
illuminant must be out of colorimetry.MSDS_CMFS['XXX']
If there is no illuminant or it has the wrong form,
the illuminant SDS_ILLUMINANTS['E']
Copy link
Member

Choose a reason for hiding this comment

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

This should be a TODO above the function, e.g.

# TODO: Implement support for passing the CMFS.

Copy link
Author

Choose a reason for hiding this comment

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

Could be implemented but is not yet.

Copy link
Member

Choose a reason for hiding this comment

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

Need to describe the type too, see a typical Returns here: https://github.com/colour-science/colour/blob/develop/colour/volume/spectrum.py#L296

Copy link
Author

Choose a reason for hiding this comment

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

Is done in actual version.

Copy link
Member

Choose a reason for hiding this comment

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

those should only be the keys, see here for reference: https://github.com/colour-science/colour/blob/develop/colour/volume/spectrum.py#L302

Copy link
Author

Choose a reason for hiding this comment

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

I changed this in actual version.

Copy link
Member

Choose a reason for hiding this comment

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

Examples

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.

Copy link
Member

Choose a reason for hiding this comment

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

We are expecting the examples to be run and serve as doctests, those cannot be run. I would make sure that they actually do. The plotting code might be better in a dedicated module in colour.examples.volume.

Copy link
Author

@gutenzwerg gutenzwerg Jun 22, 2022

Choose a reason for hiding this comment

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

Good suggestion. I will do this, if my code will be implemented. Or if You say that it will be definitively implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Why parameter here?

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.

Copy link
Member

Choose a reason for hiding this comment

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

We have a special way to handle spectral arguments whilst providing good defaults, see https://github.com/colour-science/colour/blob/develop/colour/volume/spectrum.py#L348

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.

Copy link
Member

Choose a reason for hiding this comment

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

You could do that directly, especially because the colour.colorimetry.handle_spectral_arguments definition makes sure that the illuminant is aligned to the CMFS spectral shape:

X_illuminated, Y_illuminated, Z_illuminated = tsplit(cmfs.values * illuminant.values)

Copy link
Author

Choose a reason for hiding this comment

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

unfortunately it does not work this way, because ValueError: operands could not be broadcast together with shapes (471,3) (471,)

Copy link
Author

@gutenzwerg gutenzwerg Jun 22, 2022

Choose a reason for hiding this comment

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

The given code will not run, but this one:
X_illuminated, Y_illuminated, Z_illuminated = tsplit(cmfs.values) * illuminant.values
is changed in actual version.

is chosen which has no influence to the calculations,
because it is an equal-energy-spectrum

if necessary a third parameter for the
colour-matching function could easily be implemented

Returns
-------
an array of CIE -X,Y,Z - Triples for every single wavelength
in single nm - Steps in the range from 360 to 830 nm

References
----------
- cite: Wyszecki, G., & Stiles, W. S. (2000).
In Color Science: Concepts and Methods,
Quantitative Data and Formulae (pp. 181–184). Wiley.
ISBN:978-0-471-39918-6
- cite: Francisco Martínez-Verdú, Esther Perales,
Elisabet Chorro, Dolores de Fez,
Valentín Viqueira, and Eduardo Gilabert, "Computation and
visualization of the MacAdam limits
for any lightness, hue angle, and light source," J.
Opt. Soc. Am. A 24, 1501-1515 (2007)
- cite: Kenichiro Masaoka. In OPTICS LETTERS, June 15, 2010
/ Vol. 35, No. 1 (pp. 2031 - 2033)

Example
--------
from matplotlib import pyplot as plt
import numpy as np
import math
fig = plt.figure(figsize=(7,7))
ax = fig.add_axes([0,0,1,1])
illuminant = colour.SDS_ILLUMINANTS['D65']

def plot_Narrowband_Spectra (Yxy_Narrowband_Spectra):
FirstColumn = 0
SecondColumn = 1

x = Yxy_Narrowband_Spectra[...,FirstColumn]
y = Yxy_Narrowband_Spectra[...,SecondColumn]
ax.plot(x,y,'orange',label='Spectrum Loci')

x = [Yxy_Narrowband_Spectra[-1][FirstColumn],
Yxy_Narrowband_Spectra[0][FirstColumn]]
y = [Yxy_Narrowband_Spectra[-1][SecondColumn],
Yxy_Narrowband_Spectra[0][SecondColumn]]
ax.plot(x,y,'purple',label='Purple Boundary')
return()

for n in range(1, 20):
Yxy_Narrowband_Spectra = colour.XYZ_to_xy(
colour.macadam_limits(n/20, illuminant) / 100)
plot_Narrowband_Spectra (Yxy_Narrowband_Spectra)

plt.show()
"""
target_bright = target_brightness
if target_bright > 1 or target_bright < 0:
raise TypeError(
"brightness of function macadam_limits( )"
"has to be between 0 and 1"
)
standard_cfms = MSDS_CMFS["CIE 1931 2 Degree Standard Observer"]
X_cie31 = standard_cfms.values[..., 0]
Y_cie31 = standard_cfms.values[..., 1]
Z_cie31 = standard_cfms.values[..., 2]
try:
illuminant.interpolator
except AttributeError:
illuminant = SDS_ILLUMINANTS["E"]

# If there is no illuminant or it has the wrong form,
Copy link
Member

Choose a reason for hiding this comment

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

Most of those comments could be removed as they are not providing useful information and add a lot of noise when reading the code.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.

# an illuminant chosen with no influence
# If the illuminants do not match the format of the Standard Observer,
# they have to be adapted
illuminant.extrapolate(SpectralShape(360, 830, 1))
illuminant.interpolate(SpectralShape(360, 830, 1))
# The cie31 cmfs are convolved with the given illuminant
X_illuminated = X_cie31 * illuminant.values
Y_illuminated = Y_cie31 * illuminant.values
Copy link
Member

Choose a reason for hiding this comment

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

We cannot really hardcode such number here, what if the user passes a CMFS with a 400-700-5 spectral shape?

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.

Z_illuminated = Z_cie31 * illuminant.values
# Generate empty output-array
out_limits = np.zeros_like(standard_cfms.values)
# This Array has 471 entries for wavelengths from 360 nm to 830 nm
opti_colour = np.zeros_like(Y_illuminated)
# The array of optimal colours has the same dimensions like Y_illuminated
# and all entries are initially set to zero
middle_opti_colour = 235
# is a constant and not be changed. At 595nm (360 + 235)
# in the middle of the center_opti_colour-array
# be aware that counting in array-positions starts at zero
# The first optimum color has its center initially at zero
maximum_brightness = np.sum(Y_illuminated)
Copy link
Member

Choose a reason for hiding this comment

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

Y is luminance, not brightness.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.


# "integral" over Y_illuminated

def optimum_colour(width, center):
opti_colour = np.zeros(471)
Copy link
Member

Choose a reason for hiding this comment

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

We should not hardcode such value here.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.

# creates array of 471 zeros and ones which represents optimum-colours
# All values of the opti_colour-array are initially set to zero
half_width = width
center_opti_colour = center
middle_opti_colour = 235
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for the hardcoded value.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in actual version.

opti_colour[
middle_opti_colour
- half_width : middle_opti_colour
+ half_width
+ 1
] = 1
# we start the construction of the optimum color
# at the center of the opti_colour-array
opti_colour = np.roll(
opti_colour, center_opti_colour - middle_opti_colour
)
# the optimum colour is rolled to the right wavelength
return opti_colour

def bright_opti_colour(width, center, lightsource):
brightness = (
np.sum(optimum_colour(width, center) * lightsource)
/ maximum_brightness
)
return brightness

step_size = np.array([64, 32, 16, 8, 4, 2, 1])
for wavelength in range(0, 471):
Copy link
Member

Choose a reason for hiding this comment

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

This is a very slow loop by the look of it, I haven't looked in details but we should think about vectorisation.

Copy link
Author

Choose a reason for hiding this comment

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

Yes the loop is slow, but this way I need 8 loops maximum, not 256. It's the clou of the code. Vectorisation is not possible this way.

width = 127
for n in step_size:
brightness = bright_opti_colour(width, wavelength, Y_illuminated)
if brightness > target_bright or width > 234:
width -= n
else:
width += n

brightness = bright_opti_colour(width, wavelength, Y_illuminated)
if brightness < target_bright:
width += 1

rough_optimum = optimum_colour(width, wavelength)
brightness = np.sum(rough_optimum * Y_illuminated) / maximum_brightness

# in the following, the both borders of the found rough_optimum
# are reduced to get more exact results
bright_difference = (brightness - target_bright) * maximum_brightness
# discrimination for single-wavelength-spectra
if width > 0:
opti_colour = np.zeros(471)
opti_colour[
middle_opti_colour - width : middle_opti_colour + width + 1
] = 1
# instead rolling forward opti_colour, light is rolled backward
rolled_light = np.roll(
Y_illuminated, middle_opti_colour - wavelength
)
opti_colour_light = opti_colour * rolled_light
left_opti = opti_colour_light[middle_opti_colour - width]
right_opti = opti_colour_light[middle_opti_colour + width]
interpolation = 1 - (bright_difference / (left_opti + right_opti))
opti_colour[middle_opti_colour - width] = interpolation
opti_colour[middle_opti_colour + width] = interpolation
# opti_colour is rolled to right position
final_optimum = np.roll(
opti_colour, wavelength - middle_opti_colour
)
else:
final_optimum = rough_optimum / brightness * target_bright

out_X = np.sum(final_optimum * X_illuminated) / maximum_brightness
out_Y = target_bright
out_Z = np.sum(final_optimum * Z_illuminated) / maximum_brightness
triple = np.array([out_X, out_Y, out_Z])
out_limits[wavelength] = triple
return out_limits
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this implementation, I'm left wondering if we should not simply output the XYZ values from this definition which does a similar work but using Trimesh and geometrical intersection of the Rösch-MacAdam colour solid with a plane:

def plot_visible_spectrum_section(

https://colour.readthedocs.io/en/develop/_images/Plotting_Plot_Visible_Spectrum_Section.png

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking the same thing after looking at the spectrum.py code in #994

Copy link
Author

Choose a reason for hiding this comment

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

At a first glimps it seems to be a good idea. Some reasons for my code:

  1. After some hours of working with plot_visible_spectrum_section I found no way to get out the data for doing an own plott. (May be anyone else can)
  2. I found out no way to show more layers with different sections in one graph. (May be anyone else can)
  3. Installation of Trimesh is necessary
  4. It worked a lott slower than my code
  5. It is probably a bit less accurate respectively it is not possible to coose the level of accuracy
    After a revise of my code according to KelSolaars suggestions this will be possible depending on SpectralShape