-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
base: develop
Are you sure you want to change the base?
Changes from 13 commits
16ae787
47883a8
9e27d71
f204568
99872c3
308712e
6d34507
5b8593a
afc9946
170eb03
5cbd91d
d6fe7ee
b03eaf6
f3bc4fb
7296c92
f5ee2a4
388c0f6
91c45f7
6eb76d5
41a277b
977be68
9dc7145
6f306dc
ac12a6b
75ade26
463a1b2
92a1cd1
78141ce
a190c00
c67984d
a603c18
0659c32
f91801f
95a446e
38b727a
fa6485b
c1cd25a
9f28353
e58c32b
fd3a52f
5d817da
c87523b
31f2dbd
5ed84a0
cbdb8b4
d1eca3b
b5f06c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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" | ||||
|
@@ -142,3 +143,197 @@ def is_within_macadam_limits( | |||
simplex = np.where(simplex >= 0, True, False) | ||||
|
||||
return simplex | ||||
|
||||
|
||||
def macadam_limits(target_brightness, illuminant=()): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not really explain what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. target_brightness is replaced by luminance in actual code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See how we describe that kind of parameter here: https://github.com/colour-science/colour/blob/develop/colour/volume/spectrum.py#L259 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'] | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be implemented but is not yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to describe the type too, see a typical There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is done in actual version. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this in actual version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed in actual version. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why parameter here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed in actual version. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed in actual version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could do that directly, especially because the X_illuminated, Y_illuminated, Z_illuminated = tsplit(cmfs.values * illuminant.values) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The given code will not run, but this one: |
||||
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, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not hardcode such value here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto for the hardcoded value. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: colour/colour/plotting/section.py Line 479 in a412b67
https://colour.readthedocs.io/en/develop/_images/Plotting_Plot_Visible_Spectrum_Section.png There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
There was a problem hiding this comment.
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 atilluminant=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:
There was a problem hiding this comment.
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.