JuliaAttic / Color.jl

Basic color manipulation utilities.
Other
47 stars 21 forks source link

Fixes conversion to/from Ufixed* types #63

Closed kmsquire closed 9 years ago

kmsquire commented 9 years ago

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 commented 9 years ago

Coverage Status

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

coveralls commented 9 years ago

Coverage Status

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

timholy commented 9 years ago

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 commented 9 years ago

Coverage Status

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

kmsquire commented 9 years ago

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 commented 9 years ago

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

coveralls commented 9 years ago

Coverage Status

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

timholy commented 9 years ago

Looks great to me.

timholy commented 9 years ago

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.