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

Type stability regression for mean(::Array{RGB{N0f8}}) from 0.6 to 0.7 #326

Closed
Evizero opened this issue Sep 8, 2018 · 2 comments · Fixed by JuliaGraphics/ColorVectorSpace.jl#103

Comments

@Evizero
Copy link

Evizero commented Sep 8, 2018

I don't know where the isssue was introduced exactly (or where this issue should be opened), but it seems to occur in Base._mapreduce(identity, Base.add_sum, ...) somewhere.

0.6

julia> using Colors, ColorVectorSpace, FixedPointNumbers

julia> A = rand(RGB{N0f8}, 2, 2)
2×2 Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}:
 RGB{N0f8}(0.71,0.686,0.294)   RGB{N0f8}(0.871,0.322,0.667)
 RGB{N0f8}(0.369,0.055,0.478)  RGB{N0f8}(0.016,0.035,0.533)

julia> Test.@inferred mean(A)
RGB{Float64}(0.4911764705882353,0.27450980392156865,0.4931372549019607)

0.7

julia> using Colors, ColorVectorSpace, FixedPointNumbers, Statistics, Test

julia> A = rand(RGB{N0f8}, 2, 2)
2×2 Array{RGB{N0f8},2} with eltype RGB{Normed{UInt8,8}}:
 RGB{N0f8}(0.188,0.757,0.051)  RGB{N0f8}(0.922,0.871,0.012)
 RGB{N0f8}(0.62,0.184,0.69)    RGB{N0f8}(0.863,0.498,0.675)

julia> Test.@inferred mean(A)
ERROR: return type RGB{Float64} does not match inferred return type Union{RGB{Float32}, RGB{Float64}}
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] top-level scope at none:0
@kimikage
Copy link
Collaborator

kimikage commented Aug 24, 2019

Let us sort out the issue.

First of all, Colors.jl does not provide arithmetic operation methods for colors except XYZ and LMS. (cf. Support arithmetic only for linear color spaces)
So, it seems to me that the inferred return type Union{RGB{Float32}, RGB{Float64}} comes from the codes of ColorVectorSpace.jl.

Second, what is the expected return type of mean(::Array{RGB{N0f8}})?
If you mind the type stability because of the processing speed, I wonder whether RGB{Float64} is always best. Moreover, to stop using mean may be a possible measure.
If the problem is not the speed, why not explicitly convert the result into preferred type? For example,

julia> Test.@inferred convert(eltype(A), mean(A))

By the way, the return type of Colors.weighted_color_mean() is the same as the inputs, i.e. RGB{N0f8} in this case, although it may cause a precision issue.

Well, if it is a pure backward compatibility issue, it's time to give up.
Julia v0.7 (v1.0) includes many breaking changes, you know.

timholy added a commit to JuliaGraphics/ColorVectorSpace.jl that referenced this issue Aug 24, 2019
@timholy
Copy link
Member

timholy commented Aug 24, 2019

@Evizero, sorry that I failed to notice this in the crush of updating packages to Julia 1. Fixed over at JuliaGraphics/ColorVectorSpace.jl#103

timholy added a commit to JuliaGraphics/ColorVectorSpace.jl that referenced this issue Aug 24, 2019
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 a pull request may close this issue.

3 participants