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

Fixes conversion to/from Ufixed* types #63

Merged
merged 1 commit into from
Sep 18, 2014

Conversation

kmsquire
Copy link
Contributor

  • Previously, conversions were attempting to force conversion to, e.g., Lab{Ufixed8}, which is not a valid type.
  • Conversions from ColorValue{Ufixed} types to abstract ColorValues (i.e., without a specified type parameter) failed (these conversions are allowed from ColorValue{FloatingPoint} types)
  • Some conversions from ColorValue{Ufixed} to ColorValue{FloatingPoint} did not round trip well (e.g., RGB{Ufixed8} -> Lab{Float64} -> RGB{Ufixed8})
  • Added tests.

Previously:

julia> img[1]
RGB{Ufixed8}(0.035,0.106,0.208)

julia> convert(Lab{Float64}, img[1])
Lab{Float64}(9.789275714438482,3.524345941228191,-19.242882752985235)

julia> convert(RGB{Ufixed8}, Lab{Float64}(9.789275714438482,3.524345941228191,-19.242882752985235))
RGB{Ufixed8}(0.031,0.114,0.208)

julia> # ^^^^^^^^^^^^^^^^^^^^ Inexact conversion on previous line

julia> convert(Lab, img[1])
ERROR: type: Lab: in T, expected T<:FloatingPoint, got Type{UfixedBase{Uint8,8}}
 in convert at /home/kevin/.julia/v0.4/Color/src/conversions.jl:27

With this PR:

julia> convert(Lab, img[1])
Lab{Float64}(9.789275714438482,3.524345941228191,-19.242882752985235)

julia> convert(RGB{Ufixed8}, ans)
RGB{Ufixed8}(0.035,0.106,0.208)

While the behavior is obviously better, this may not be the right fix, so I would appreciate feedback.

Cc: @timholy

@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) when pulling ec3f356 on kms/Ufixed_conversion_fixes into 3aebbb8 on master.

_convert{CV<:AbstractRGB}(::Type{CV}, c::DIN99) = _convert(CV, convert(XYZ{eltype(c)}, c))
_convert{CV<:AbstractRGB}(::Type{CV}, c::DIN99o) = _convert(CV, convert(XYZ{eltype(c)}, c))
_convert{CV<:AbstractRGB}(::Type{CV}, c::DIN99d) = _convert(CV, convert(XYZ{eltype(c)}, c))
_convert{CV<:AbstractRGB}(::Type{CV}, c::LMS) = _convert(CV, convert(XYZ{eltype(c)}, c))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the change I'm least sure about.

Previously, when converting to, e.g., RGB{Ufixed8}, the first conversion was to, e.g., XYZ{Ufixed8}, followed by a conversion to RGB{Ufixed8}. This caused a loss of precision, or failure (e.g., with Lab{Ufixed8}).

Now, the function will convert, e.g., xyY{Float32} to XYZ{Float32}, then to RGB{Float32}, presumably without much loss of precision (but possibly some in the least significant digits). Would it be better to use Float64 directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt Float64 is necessary. Our dynamic range of color perception does not extend to 16 decimals of precision. Probably Float16 is sufficient...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, presumably eltype(c) is sufficient here?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) when pulling 402cee1 on kms/Ufixed_conversion_fixes into 3aebbb8 on master.

@timholy
Copy link
Contributor

timholy commented Sep 17, 2014

Thanks for this, it's obviously a step forward. Getting the logic of these conversions right, with so many different types, is surprisingly tricky. Thanks for cleaning up my mess!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.42%) when pulling bfb34ed on kms/Ufixed_conversion_fixes into 3aebbb8 on master.

@kmsquire
Copy link
Contributor Author

NP! I had actually worked my way here from Images, after trying

julia> convert(Image{Lab}, img)
ERROR: type: Lab: in T, expected T<:FloatingPoint, got Type{UfixedBase{Uint8,8}}
 in _convert at /home/kevin/.julia/v0.4/Images/src/core.jl:204
 in convert at /home/kevin/.julia/v0.4/Images/src/core.jl:200

You have some pretty convoluted code in there to try to mimic triangle dispatch. :-)

I worked through it somewhat, but it looks like the Color array conversion should be moved to this package, and then dealt with in a similar way to this patch. Thoughts?

@kmsquire
Copy link
Contributor Author

I've updated according to your comments, Tim. If there's nothing else, I'll squash and merge in a little while.

* Previously, conversions were attempting to force conversion to,
  e.g., HSV{Ufixed8}, which is not a valid type
@coveralls
Copy link

Coverage Status

Coverage increased (+0.42%) when pulling b558a50 on kms/Ufixed_conversion_fixes into 3aebbb8 on master.

@timholy
Copy link
Contributor

timholy commented Sep 18, 2014

Looks great to me.

@timholy
Copy link
Contributor

timholy commented Sep 18, 2014

Yes, the type handling was easily the trickiest part about the recent migration. If there had been an easier syntax for triangular dispatch, it would indeed have been simpler.

Regarding stuff to move here, are you referring to some of the functions in Images/src/core.jl (reinterpret and convert), or to stuff in Images/src/colortypes.jl? I think I'm not seeing the precise material you're describing. But overall I agree it's not clear what should be where, and certainly more stuff should probably come here.

kmsquire added a commit that referenced this pull request Sep 18, 2014
@kmsquire kmsquire merged commit 5ce3577 into master Sep 18, 2014
@kmsquire kmsquire deleted the kms/Ufixed_conversion_fixes branch September 18, 2014 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants