JuliaGraphics / ColorTypes.jl

Basic color definitions and traits
Other
78 stars 36 forks source link

Simplify the implementation of eltype #212

Closed timholy closed 4 years ago

timholy commented 4 years ago

I'm guessing the old implementation predated Julia 0.6 and the new type system.

codecov[bot] commented 4 years ago

Codecov Report

Merging #212 into master will decrease coverage by 0.01%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
- Coverage   94.10%   94.09%   -0.02%     
==========================================
  Files           8        8              
  Lines         696      694       -2     
==========================================
- Hits          655      653       -2     
  Misses         41       41              
Impacted Files Coverage Δ
src/traits.jl 97.81% <100.00%> (-0.03%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6d2759c...ec75e9e. Read the comment docs.

timholy commented 4 years ago

Yep, still returns Any. eltype(RGB) dispatches to this method. If that seems OK to you, then I don't think this changes behavior.

Alternatively, the implementation

eltype(::Type{C}) where {C<:Colorant{T}} where {T} = T
eltype(::Type{C}) where {C<:Colorant} = Base.parameter_upper_bound(C, 1)

returns the "valid" type as implied in #151. Thoughts? EDIT: oh, I see I even mentioned that in https://github.com/JuliaGraphics/ColorTypes.jl/issues/151#issuecomment-575583648, but I don't think we really decided.

kimikage commented 4 years ago

oh, I see I even mentioned that in #151 (comment), but I don't think we really decided.

Yes, and we now have the problem with parameter_upper_bound(cf. https://github.com/JuliaGraphics/ColorTypes.jl/issues/208). If we use _parameter_upper_bound, the recursion still seems to be a necessary technique.

Until we make a decision about PR #184, we should not change the current behavior.

timholy commented 4 years ago

Interestingly, this implementation is much friendlier to precompilation of ImageCore methods.