Skip to content

promote_rule for Type{Any} #369

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

Closed
johnnychen94 opened this issue Dec 1, 2019 · 9 comments
Closed

promote_rule for Type{Any} #369

johnnychen94 opened this issue Dec 1, 2019 · 9 comments

Comments

@johnnychen94
Copy link
Member

I'm expecting the following codes work

mask = zeros(Gray, 2, 2) # Gray{Any}
mask[1, 2] = 1
rgb_img = rand(RGB{N0f8}, 2, 2) # RGB{N0f8}
masked_img = Bool.(mask) .* rgb_img # RGB{N0f8}

hcat(mask, masked_img) 

But it throws some disgusting error 😥

ERROR: TypeError: in RGB, in T, expected T<:Union{AbstractFloat, FixedPoint}, got Type{Any}
Stacktrace:
 [1] promote_rule at C:\Users\johnn\.julia\packages\Colors\r1p4Q\src\promotions.jl:1 [inlined]
 [2] promote_type at .\promotion.jl:223 [inlined]
 [3] promote_eltype(::Array{Gray,2}, ::Array{RGB{Normed{UInt8,8}},2}) at .\abstractarray.jl:1268
 [4] hcat(::Array{Gray,2}, ::Array{RGB{Normed{UInt8,8}},2}) at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.3\SparseArrays\src\sparsevector.jl:1075
 [5] top-level scope at REPL[131]:1

P.S. This can be worked around if we do mask = zeros(Gray{N0f8}, 2, 2)

@kimikage
Copy link
Collaborator

kimikage commented Dec 5, 2019

I guess this is a problem with zeros rather than promote_rule. In principle, C{Any}s should not be manipulated directly by users.

julia> zeros(Gray, 2, 2)
2×2 Array{Gray{Any},2} with eltype Gray:
 Gray{N0f8}(0.0)  Gray{N0f8}(0.0)
 Gray{N0f8}(0.0)  Gray{N0f8}(0.0)

julia> zeros(AbstractGray, 2, 2)
ERROR: MethodError: no method matching zero(::Type{Color{T,1} where T})

julia> zeros(RGB, 2, 2)
ERROR: MethodError: no method matching zero(::Type{RGB})

Your workaround is a "correct" manner. If you don't want to use abstract color types but just want the simple notation, zeros should return the same type as rand returns. Probably, it is responsibility of ColorVectorSpace for now.

julia> using ColorVectorSpace

julia> zeros(RGB, 2, 2)
2×2 Array{RGB{Any},2} with eltype RGB:
 RGB{N0f8}(0.0,0.0,0.0)  RGB{N0f8}(0.0,0.0,0.0)
 RGB{N0f8}(0.0,0.0,0.0)  RGB{N0f8}(0.0,0.0,0.0)

@kimikage
Copy link
Collaborator

kimikage commented Dec 5, 2019

@kimikage
Copy link
Collaborator

Using the new promotion rules implemented in ColorTypes.jl, we can get the following result without errors. Damn!

julia> hcat(mask, masked_img)
2×4 Array{RGB{Any},2} with eltype RGB:
 RGB{N0f8}(0.0,0.0,0.0)  RGB{N0f8}(1.0,1.0,1.0)  RGB{N0f8}(0.0,0.0,0.0)  RGB{N0f8}(0.541,0.349,0.294)
 RGB{N0f8}(0.0,0.0,0.0)  RGB{N0f8}(0.0,0.0,0.0)  RGB{N0f8}(0.0,0.0,0.0)  RGB{N0f8}(0.0,0.0,0.0)

I'm sorry that no error is given, but I think this is a "reasonable" result.:cry:

So, this is just a problem with zeros now.

@johnnychen94
Copy link
Member Author

johnnychen94 commented Mar 1, 2020

Given that JuliaLang/julia#34948 is basically disapproved, is there anything that needs to be fixed in this issue?

💤

@kimikage
Copy link
Collaborator

kimikage commented Mar 1, 2020

As you know, the array with non-concrete element type has the performance issue. I think your suggestions are good for application specific types.

Originally posted by @kimikage in JuliaGraphics/ColorTypes.jl#175 (comment)

I think that the return type of ones(Gray) should be decided in terms of image processing.

Originally posted by @kimikage in JuliaGraphics/ColorTypes.jl#175 (review)

@timholy
Copy link
Member

timholy commented Mar 1, 2020

If we decide to make * mean elementwise multiplication (JuliaGraphics/ColorVectorSpace.jl#38, JuliaGraphics/ColorVectorSpace.jl#109, JuliaGraphics/ColorVectorSpace.jl#119), then we could go back to having one(::Type{C}) where {C<:Union{AbstractGray,AbstractRGB}} = oneunit(C) (if ColorVectorSpace is loaded).

@kimikage
Copy link
Collaborator

kimikage commented Mar 1, 2020

This is off topic, but CSS Color Module Level 4 will introduce a new color notation gray() (cf. https://www.w3.org/TR/css-color-4/#grays)
Note that this "gray" means "L" in Lab space. Furthermore, the white point in the Lab space is D50 instead of D65, and the white point transformation uses the Bradford transform instead of CAT02.
If I add the support for the CSS4 "gray()" with Colors.jl's parser, the intensity will be converted to sRGB in the specified manner, and the return value will be in RGB{N0f8}.

Anyway, the facts that a gray is:

  • equivalent to an RGB color where R = G = B
  • in the range [0,1]
  • linear

are no longer "universal". So I don't really welcome bringing many rules about gray into ColorTypes.jl.

@timholy
Copy link
Member

timholy commented Mar 1, 2020

Ugh. I vote we all mutate our L and S cones and go back to seeing the world in grayscale.

@kimikage
Copy link
Collaborator

The problem with zeros/ones remains, but in any case, there seems to be no discussion "here".

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