RGB(2.0,0.0,0.0) should throw an error

[dlfivefifty]

I was surprised that the following different throw an error:

RGB(2.0,0.0,0.0)

This ended up causing an error further down in a plotting package https://github.com/JuliaPlots/Plots.jl/issues/851#issuecomment-302924994. Is this intentional?

@mkborregaard

[SimonDanisch]

You mean, that this doesn't error? I think it's intentional to do math with the colors. You need to clamp it before converting it to a storage type that only allows the range of 0-1! You can use ImagesCore.clamp01, or mapc(x-> clamp(x, 0, 1), RGB(...))

[mkborregaard]

Why not have an internal constructor in RGB that checks that the arguments are in the permitted range? I know the test may take a small amount of time that you may not appreciate when analyzing huge MRI images - but then possibly there should be an unsafe_RGB type for these uses? At least it seems strange that the 0,1 range is documented in the docstring but not enforced in the code, and downstream it's nice to be able to assume that an RGB has values bounded by 0 and 1.

[SimonDanisch]

It's not only about performance though, it's about processing the colors... Which becomes pretty annoying, if the default doesn't allow to exit the range of 0 and 1! In contrast, it's pretty straight forward to include one clamp at your last step of processing.

[dlfivefifty]

Maybe the spec should be that RGB(2.0,0.0,0.0) should be treated like RGB(1.0,0.0,0.0).

[mkborregaard]

But that's what clamp does, right? But @SimonDanisch what is the 'last step of processing'? I think an unsafe_RGB for maths processing that allows deviations and a safe RGB (with clamp in the internal constructor) for passing to applications that assume RGB values to be valid should make that very clear - the last step is the one generating the safe RGB? Anyway I am not saying that this is the best design, just that passing invalid RGB values downstream to a naive application that assumes that fields can be treated as 0-1 values can be the cause of all kinds of weird errors.

[mkborregaard]

On my system:

julia> type foo
       bar::Float64
       end

julia> type baz
       bar::Float64
       baz(x) = new(clamp(x,0,1))
       end

julia> using BenchmarkTools

julia> @benchmark foo(0.5)
BenchmarkTools.Trial:
  memory estimate:  16 bytes
  allocs estimate:  1
  --------------
  minimum time:     6.823 ns (0.00% GC)
  median time:      7.031 ns (0.00% GC)
  mean time:        9.044 ns (9.77% GC)
  maximum time:     1.592 μs (97.46% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @benchmark baz(0.5)
BenchmarkTools.Trial:
  memory estimate:  16 bytes
  allocs estimate:  1
  --------------
  minimum time:     9.169 ns (0.00% GC)
  median time:      9.292 ns (0.00% GC)
  mean time:        11.202 ns (9.92% GC)
  maximum time:     1.719 μs (97.71% GC)
  --------------
  samples:          10000
  evals/sample:     999

julia> @benchmark baz(1.5)
BenchmarkTools.Trial:
  memory estimate:  16 bytes
  allocs estimate:  1
  --------------
  minimum time:     9.164 ns (0.00% GC)
  median time:      9.300 ns (0.00% GC)
  mean time:        11.227 ns (10.23% GC)
  maximum time:     1.902 μs (97.90% GC)
  --------------
  samples:          10000
  evals/sample:     999

So there is a small performance drop (~25% in this case) but may be worth it.

[m-lohmann]

@mkborregaard: what @SimonDanisch means is that clamping is pretty much the worst thing you can do to RGB data in terms of color fidelity and color metrics.

We don’t even have explicitely specified which RGB space we are using. Now imagine you are using an Adobe RGB image, which has a larger gamut than the more common sRGB standard. Now you try to convert your ARGB image to sRGB which most likely might lead to illegal colors in sRGB. If you just clip them the image looks plain horrible. If you use other gamut conversion methods like in Photoshop you can reach a perceptually much more convincing result. Simple clipping leads to much larger projection errors. Furthermore, “illegal” colors might be an intermediate result of working in gamuts like CIELAB etc. which only should be corrected to the final device dependent gamut like sRGB in the last step of the color management pipeline.

[timholy]

We already have this:

julia> RGB(2,2,2)
ERROR: ArgumentError: (2, 2, 2) are integers in the range 0-255, but integer inputs are encoded with the N0f8
  type, an 8-bit type representing 256 discrete values between 0 and 1.
  Consider dividing your input values by 255, for example: RGB{N0f8}(2/255,2/255,2/255)
  See the READMEs for FixedPointNumbers and ColorTypes for more information.
Stacktrace:
 [1] throw_colorerror(::Type{FixedPointNumbers.Normed{UInt8,8}}, ::Tuple{Int64,Int64,Int64}) at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:655
 [2] throw_colorerror(::Type{FixedPointNumbers.Normed{UInt8,8}}, ::Int64, ::Int64, ::Int64) at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:624
 [3] checkval at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:606 [inlined]
 [4] ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}}(::Int64, ::Int64, ::Int64) at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:90
 [5] ColorTypes.RGB(::Int64, ::Int64, ::Int64) at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:441

But as I've pointed out elsewhere, you need to be able to create out-of-gamut values for computing mean(img) for which the only reasonable algorithm is sum(img)/length(img).

[mkborregaard]

Thanks for the explanation @m-lohmann , that makes a lot of sense. I think I already understood that from @SimonDanisch 's explanation above. Still, the talk about the 'last step' (and the docstring specifying the 0-1 range) gives me the impression that there are some aspects of RGB that are useful for internal image reprocessing in a package, whereas others are more useful for downstream end user packages that want to just assume that RGB values are valid. Hence my suggestion for the safe and unsafe types.

To be specific: Plots throws an error on some backends when invalid RGB values are supplied, but other backends don't: JuliaPlots/Plots.jl#851 That is because the color preprocessing is different between packages, though all work for valid colors.

This is an unexpected result, and in addition the error message is unhelpful, making the issue hard to debug. If everything in ColorTypes is specialized for internal image reprocessing, what is then the advice for other downstream uses of color?

EDIT: I posted this before seeing @timholy 's response above, but I think the comment still stands.

[timholy]

If you want to explore safe and unsafe types, I'd be willing to evaluate a PR. But simply passing the tests for this package wouldn't be good enough: you'd need to check your new types against Images.jl and quite a few other packages (at last count there were more than 50 packages that are dependents of this one).

Fair warning: this might not be a small amount of work.

[SimonDanisch]

You could just make sure that Plots.jl passes valid colors to the backend?

[dlfivefifty]

Maybe the simplest solution is if Plots clamps the values before passing to the backends?

[timholy]

Still, the talk about the 'last step' (and the docstring specifying the 0-1 range) gives me the impression that there are some aspects of RGB that are useful for internal image reprocessing in a package, whereas others are more useful for downstream end user packages that want to just assume that RGB values are valid.

It's more an issue that colorspaces are complicated, and this package is about more than just RGB.

[timholy]

Maybe the simplest solution is if Plots clamps the values before passing to the backends?

Better would be convert(RGB{N0f8}, c). That will catch any out-of-range values and have it throw an error.

[mkborregaard]

I can do that, yes. I'd definitely prefer editing Plots to making a PR on the entire Images ecosystem. I'd say I don't find the suggestion that I do that (alone) very fair, as I and @dlfivefifty are pointing out a real problem, though it does have valid reasons to be the way it is.

My question in the last post was "what is the suggestion for downstream packages". Now, and before, I am willing to make the accomodation in Plots if the advice is "downstream packages should always check the validity of colors". I don't find that approach ideal, though, as Plots is not the only package that works with colors.

[timholy]

Sorry, but there's some history here. The last time this package needed a big refresh, I donated an entire week of my time to satisfy the various needs and to make sure the changes didn't break the ecosystem. And I was really, really grumpy about it. https://github.com/JuliaArchive/Color.jl/issues/101. I still feel bad about being so grumpy, but I can assure you it wasn't a fun experience.

[mkborregaard]

I can absolutely sympathize with that!

[mkborregaard]

Last question - N0f8 is that FixedPointNumbers.Normed{UInt8,8} ?

[timholy]

Yes. I've tried to make sure that any conversion error is quite informative:

julia> convert(RGB{N0f8}, RGB(2.0,0,0))
ERROR: ArgumentError: element type FixedPointNumbers.Normed{UInt8,8} is an 8-bit type representing 256 values from 0.0 to 1.0,
  but the values (2.0, 0.0, 0.0) do not lie within this range.
  See the READMEs for FixedPointNumbers and ColorTypes for more information.
Stacktrace:
 [1] throw_colorerror_(::Type{FixedPointNumbers.Normed{UInt8,8}}, ::Tuple{Float64,Float64,Float64}) at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:632
 [2] throw_colorerror(::Type{FixedPointNumbers.Normed{UInt8,8}}, ::Float64, ::Float64, ::Float64) at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:624
 [3] checkval at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:606 [inlined]
 [4] ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}}(::Float64, ::Float64, ::Float64) at /home/tim/.julia/v0.6/ColorTypes/src/types.jl:90
 [5] convert(::Type{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}}}, ::ColorTypes.RGB{Float64}) at /home/tim/.julia/v0.6/ColorTypes/src/conversions.jl:7

so hopefully it will be much easier for you to debug.

[timholy]

(Beats the heck out of InexactError(), which is what it used to be.)

[mkborregaard]

InexactError is in fact what brought @dlfivefifty and I here :-) Thanks! I'll change the code in Plots.

[timholy]

Clearly I, or someone, needs to get back to https://github.com/JuliaLang/julia/pull/20005.

[timholy]

Well, it's been an incredibly long road (it included rewriting the Julia compiler's inlining engine and replacing some intrinsics, https://github.com/JuliaLang/julia/pull/22210 and https://github.com/JuliaLang/julia/pull/22202), but InexactError is no longer such a difficult error to debug. https://github.com/JuliaLang/julia/pull/20005 is merged.

[mkborregaard]

Awesome! I'm guessing this issue can be closed?