Conversion methods should be organized

[kimikage]

I think the set of conversion methods in src/conversions.jl is too complicated. What makes matter more complicated is that they work together with the conversion methods in ColorTypes.jl.

One of the problems is that there are at least two stray methods which are not called by convert()(cf. PR #352). The codecov suggests that this have also strayed.

I think it is a more serious issue that very few maintainers know the entire structure of conversions. Of course, I do not know! I have a plan to add a optional argument wp (white point) to convert()(cf. issue #278 and PR #340), but I don't know what it really means.

However, I have no clear solution. It would be better to make the coding guideline and refactor the codes.

What should we do?

[timholy]

I am not quite sure what type of complexity you are referring to: there are several candidates!

The conversion methods in ColorTypes are "trivial" in the sense that they don't change color spaces, only representations in the same color space. They are also insanely complicated from the dispatch perspective because of handling stuff like convert(RGB, c) rather than forcing the user to specify convert(RGB{Float32}, c). At the time ColorTypes was developed, Julia's optimizer was nowhere near as good as it is now, so the effort I had to expend to make this work was pretty crazy. Now with good constant-propagation etc, it's quite possible that this could all be replaced by far more "obvious" design patterns.

For whitepoint, you know more than I do, so let me ask: would it make more sense to specify the whitepoint as part of the type itself? That is to say, "convert c to RGB assuming this particular whitepoint" really means "RGB-with-whitepoint"? If so, we might want to switch to the syntax

convert(WhitePoint{RGB{N0f8}}(wp), c)

Another alternative would be a keyword argument. Again, these are far faster (still not cost-free, but pretty close) than they were when this package was originally designed.

I suppose a third sense of complexity might be the question of whether you have direct C1->C2 conversions or always go through C0, C1->C0->C2. The latter requires 2n conversion methods (n being the number of types) whereas the former can get as high as n^2 methods.

[kimikage]

Well, I'm never a color expert, and the experts may say different things on their standpoints in different fields(e.g. colormetry, color management, graphic design, psychology, neurology, illuminating engineering, television, ...).

For white point, I think WhitePoint{RGB{N0f8}} is not so good, because the inclusion relation does not match the facts and it is only useful in very limited situations.

The relationship between color types and white points is not simple. The color spaces can be roughly divided into two, i.e. "absolute" and "non-absolute". Well, "what does absolute mean?", the experts may ask. It is not simple, too.
Let's define the XYZ color space as an absolute color space. As far as I know, there is no common definition other than CIE-defined XYZ (CIEXYZ). On the other hand, RGB has several popular definitions (e.g. sRGB, AdobeRGB, DCI-P3). However, the sRGB is an absolute color space because the space can project to the absolute XYZ color space by defining its condition about the white point (D65). Similarly, the Lab (CIELAB) color space becomes an absolute color space by specifying a white point. For instance, the PCSLAB (Profile Connection Space LAB) in ICC profile is an absolute space because it use D50 (not D65).

Such being the case, we can formally convert colors from CIELAB (D65) to sRGB (D65) via CIEXYZ. Yes, the WhitePoint{T} may help this. However, although both sRGB and AdobeRGB use the D65, they use different gamma values and have different gamuts (see also #8 for the gamma issues). So, WhitePoint{RGB} is useless for the ambiguous RGB.

Even though defining new concrete types of RGB (e.g AdobeRGB, P3RGB) may solve this problem (I think we must not do it in ColorTypes.jl), there is another problem related to the conversion between different white points, where my concern is. Although it is formally possible to re-convert the XYZ values to Lab with D50, which are converted from RGB with D65, it is semantically or perceptually incorrect, because our visual system has the mechanism of chromatic adaptation.
Thus, we have to specify not only src/dest white points but the CAT (Chromatic Adaptation Transform) model. Moreover, the white point is an element of viewing conditions, and there are different white points (e.g. background, surround and proximal-field). I don't love CAT_BFD{WhitePoint{RGB{N0f8}}}! 😈
It is an option to put the src/dest white points into the CAT object as I mentioned here (interface plan 4.).

Let's suppose we can solve the problem of the chromatic adaptation. We still have another problem with the gamut. Sometimes a color which is included in the source gamut is not in the destination gamut. Perhaps someone may want to specify the rendering intent (e.g. perceptual, absolute, saturation), and the person would like to specify the black point.

I feel I said too much negatively, but the color conversion is a profound matter. Let's wait for someone to create ColorManagement.jl! 😇

[kimikage]

Sorry for ranting. In conclusion, I'm not so pessimistic about the issues of context (e.g. white points) needed for the conversion. The reasons are as follows.

• It is clear that the context is too large for the type system to handle.
• If we don't use global variables (I think we should not use), all we do is add arguments.
• The only public interface is convert(), and the inside can be modified from time to time based on tests and benchmarks.

So, the type of complexity which I'm mainly referring is related to the tests and benchmarks. (Of course, the other aspects are also important as long as we are on track.)

I think the n^2 methods or test cases do not matter as far as we count a set of the eltype variants Foo{T} as n = 1.
However, the lack of unity or specialization with the eltype can be seen in some places.
For example:

Colors.jl/src/conversions.jl

Lines 328 to 360 in d64fb1a

	cnvt(::Type{XYZ{T}}, c::Lab) where {T} = convert(XYZ{T}, c, WP_DEFAULT)
	cnvt(::Type{XYZ{T}}, c::LCHab) where {T} = cnvt(XYZ{T}, convert(Lab{T}, c))
	cnvt(::Type{XYZ{T}}, c::DIN99) where {T} = cnvt(XYZ{T}, convert(Lab{T}, c))
	cnvt(::Type{XYZ{T}}, c::DIN99o) where {T} = cnvt(XYZ{T}, convert(Lab{T}, c))
	cnvt(::Type{XYZ{T}}, c::LCHab) where {T<:Normed} = cnvt(XYZ{T}, convert(Lab{eltype(c)}, c))
	cnvt(::Type{XYZ{T}}, c::DIN99) where {T<:Normed} = cnvt(XYZ{T}, convert(Lab{eltype(c)}, c))
	cnvt(::Type{XYZ{T}}, c::DIN99o) where {T<:Normed} = cnvt(XYZ{T}, convert(Lab{eltype(c)}, c))
	function xyz_to_uv(c::XYZ)
	d = c.x + 15c.y + 3c.z
	d==0 && return (d, d)
	u = 4c.x / d
	v = 9c.y / d
	return (u, v)
	end
	function cnvt(::Type{XYZ{T}}, c::Luv, wp::XYZ = WP_DEFAULT) where T
	(u_wp, v_wp) = xyz_to_uv(wp)
	a = (52 * (c.l==0 ? zero(T) : c.l / (c.u + 13 * c.l * u_wp)) - 1) / 3
	y = c.l > xyz_kappa * xyz_epsilon ? wp.y * ((c.l + 16) / 116)^3 : wp.y * c.l / xyz_kappa
	b = -5y
	d = y * (39 * (c.l==0 ? zero(T) : c.l / (c.v + 13 * c.l * v_wp)) - 5)
	x = d==b ? zero(T) : (d - b) / (a + 1/3)
	z = a * x + b + zero(T)
	XYZ{T}(x, y, z)
	end
	cnvt(::Type{XYZ{T}}, c::LCHuv) where {T} = cnvt(XYZ{T}, convert(Luv{T}, c))

• Why is cnvt(::Type{XYZ{T}}, c::Lab) where {T} defined here? The Luv version uses the optional argument wp::XYZ = WP_DEFAULT.
• Why do not LCHab and so on have wp argument?
• Why is there no Normed specialization for LCHuv?
  • ColorTypes.jl defines XYZ{T<:AbstractFloat} <: Color{T,3} (not XYZ{T<:Fractional} ).

(I do not intend to examine the individual reasons here.)
As a general rule, when we make modifications, we should check the effect by testing but cannot 100% inspection.

It is relatively easy to find out which methods are called by convert() with a particular parameter set, but it is not obvious what calls a particular method.

[kimikage]

The following is the outline of cnvt graph without the consideration of element types (generated by Graphviz).

# DOT language
digraph "cnvt" {
	graph [layout=neato overlap=false nodesep=0.08 bgcolor=transparent]
	edge [color=dimgray]
	node [width=1 height=0.5 shape=oval style=filled fillcolor="#f0f0f0" color=lightgray]
	subgraph "hint" {
		edge [style=invis]
		HSV->HSL->HSI; YIQ->YCbCr; LCHab->LCHuv; DIN99->DIN99d
	}
	RGB, Lab, Luv [fillcolor="#f8f8f8"]
	XYZ [fillcolor="#ffffff"]

	{HSV, HSL, HSI, YIQ, YCbCr}->RGB
	LCHab->Lab /*->XYZ->RGB */ [color=goldenrod]
	LCHuv->Luv /*->XYZ->RGB */ [color=goldenrod]
	/* Color3->*/ XYZ->RGB [color=steelblue style="bold"]

	/* Color3->*/ RGB->HSV [color=steelblue style="bold"]

	/* Color3->*/ RGB->HSL [color=steelblue style="bold"]

	/* Color3->*/ RGB->HSI [color=steelblue style="bold"]

	{RGB, xyY, Lab, Luv, DIN99d, LMS}->XYZ
	{HSV, HSL, HSI, YIQ, YCbCr}->RGB->XYZ [color=brown]
	{LCHab, DIN99, DIN99o}->Lab->XYZ [color=brown]
	LCHuv->Luv->XYZ [color=brown]

	/* Color3->*/ XYZ->xyY [color=steelblue style="bold"]

	{LCHab, DIN99, DIN99o}->Lab
	RGB->XYZ /*->Lab*/ [color=goldenrod]
	HSV->RGB /*->XYZ->Lab*/ [color=goldenrod]
	HSL->RGB /*->XYZ->Lab*/ [color=goldenrod]
	/* Color3->*/ XYZ->Lab [color=steelblue style="bold"]

	LCHuv->Luv
	RGB->XYZ /*->Luv*/ [color=goldenrod]
	HSV->RGB /*->XYZ->Luv*/ [color=goldenrod]
	HSL->RGB /*->XYZ->Luv*/ [color=goldenrod]
	/* Color3->*/ XYZ->Luv [color=steelblue style="bold"]

	/* Color3->*/ Luv->LCHuv [color=steelblue style="bold"]

	/* Color3->*/ Lab->LCHab [color=steelblue style="bold"]

	/* Color3->*/ Lab->DIN99 [color=steelblue style="bold"]

	/* Color3->*/ XYZ->DIN99d [color=steelblue style="bold"]

	/* Color3->*/ Lab->DIN99o [color=steelblue style="bold"]

	/* Color3->*/ XYZ->LMS [color=steelblue style="bold"]

	/* Color3->*/ RGB->YIQ [color=steelblue style="bold"]

	/* Color3->*/ RGB->YCbCr [color=steelblue style="bold"]
}

Since there is no loop, the conversion path is uniquely determined by source and destination types. The conversions where the destination is XYZ (i.e. cnvt(::Type{XYZ{T}}, c)), may need to specify the route (i.e. via RGB, Lab or Luv) as the red (brown) edges show. On the other hand, the routes of the other conversions are obvious. However, there are conversion methods which specify the obvious route as the yellow (goldenrod) edges show.
In other words, we need to pay attention to the following eight methods:

Colors.jl/src/conversions.jl

Lines 168 to 169 in d64fb1a

	cnvt(::Type{CV}, c::LCHab) where {CV<:AbstractRGB} = cnvt(CV, convert(Lab{eltype(c)}, c))
	cnvt(::Type{CV}, c::LCHuv) where {CV<:AbstractRGB} = cnvt(CV, convert(Luv{eltype(c)}, c))

Colors.jl/src/conversions.jl

Lines 417 to 419 in d64fb1a

	cnvt(::Type{Lab{T}}, c::AbstractRGB) where {T} = convert(Lab{T}, convert(XYZ{T}, c))
	cnvt(::Type{Lab{T}}, c::HSV) where {T} = cnvt(Lab{T}, convert(RGB{T}, c))
	cnvt(::Type{Lab{T}}, c::HSL) where {T} = cnvt(Lab{T}, convert(RGB{T}, c))

Colors.jl/src/conversions.jl

Lines 517 to 519 in d64fb1a

	cnvt(::Type{Luv{T}}, c::AbstractRGB) where {T} = cnvt(Luv{T}, convert(XYZ{T}, c))
	cnvt(::Type{Luv{T}}, c::HSV) where {T} = cnvt(Luv{T}, convert(RGB{T}, c))
	cnvt(::Type{Luv{T}}, c::HSL) where {T} = cnvt(Luv{T}, convert(RGB{T}, c))

Although further check is required, the tests were passed even when I eliminated the methods.

[kimikage]

An error occurs when converting an RGB24 color to Gray.

julia> convert(Gray, RGB24(1,0,0))
ERROR: type RGB24 has no field r

Colors.jl/src/conversions.jl

Lines 783 to 787 in d64fb1a

	function convert(::Type{Gray{T}}, x::AbstractRGB{T}) where {T<:Normed}
	TU = FixedPointNumbers.rawtype(T)
	val = min(typemax(TU), 0.299f0*reinterpret(x.r) + 0.587f0*reinterpret(x.g) + 0.114f0*reinterpret(x.b))
	return Gray{T}(T(round(TU, val), 0))
	end

This may not be an important issue, but this is a good example to show the importance of the unveiling.
I think we should avoid the heavy use of multiple dispatch for the public convert() .

BTW, I think there is no need for the min.

Edit:
There is a similar problem with conversion to Gray24 because Gray24 is not a subtype of Gray, but a subtype of AbstractGray.

julia> convert(Gray24, RGB())
ERROR: StackOverflowError:

[kimikage]

I think the conversion methods should be organized, but that may not actually be true.
I wanted to organize them, not clean them up.

I will report separate issues for the bugs, and I will close this issue.

[kimikage]

I understand that the issue report is not my diary, but....

I wanted to incorporate the opinions of many developers and users for major renovations (cf. #278). However, I realized that this method took too long. I think the issue reports which lost their freshness are obstacles to development, and the issues written on the second or subsequent pages on GitHub are especially not good.

I thought it would be more beneficial to fix individual defects than to spoil the issue reports for a pie in the sky. So, I decided to close this issue and #278.

In that context, the PR #380 is exactly what I wanted, and I have no reason to refuse it. However, its code is going the opposite of what I was aiming for. To be honest, as an engineer, I am not confident in this decision.

[timholy]

In what way is it going in the opposite direction? (A link is fine if you've already explained it once or more than once.) EDIT: let me guess, you mean because it goes through the bottleneck of RGB? I think we should fix the bug without that getting in the way of a more ambitious overhaul.

[kimikage]

In what way is it going in the opposite direction?

I think the set of conversion methods in src/conversions.jl is too complicated. What makes matter more complicated is that they work together with the conversion methods in ColorTypes.jl.

I think we should avoid the heavy use of multiple dispatch for the public convert() .

My concern is that the gray conversions are out of the cnvt graph, rather than their redundancy.
cf. master...kimikage:cnvt (stale)

Of course, the bug fixes are nice. 👍