search

JuliaAttic / Color.jl

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

Fixing Coloramity #101

Closed timholy closed 9 years ago

timholy commented 9 years ago

In an ideal world, one would be able to:

Unfortunately, currently we are far from living in this world. It seems that the biggest problem is the separation between this package (Color) and ColorTypes.jl. I'm running into this in the course of trying to split out functionality in Images into the JuliaIO packages. Overall I agree that splitting this functionality is a good thing: for example, it would be great to largely divorce Images.jl from ImageMagick. The problem is that the ImageMagick.jl package started by Simon (using code lifted from Images) uses ColorTypes, while Images (and essentially all other packages) uses Color, so the GL&JuliaIO world is completely incompatible with the rest of the package ecosystem.

@rant begin
    I'd like to propose that henceforth if one snips out a chunk of functionality
    (especially, types) from one popular package into a standalone package, it's
    incumbent on the programmer to issue pull requests rebasing the
    "parent" package on the new standalone package. This includes getting
    all the parent's tests working. If you're not willing to do that, it's better to use
    the parent package unmodified.
end

I have a proposal for fixing this, and I'm (grumpily) willing to put in the (considerable) time needed to make it happen:

CC @lobingera, @timothyrenner, @shashi, @dcjones, @vtjnash, @stevengj, @nolta, @simondanisch, @jiahao. I'd like feedback from most/all of you before undertaking this course, since it will be a fair amount of work and I'd like to have some confidence that the PR I submit will be accepted.

Proposed type hierarchy sketch for the new ColorTypes:

using FixedSizeArrays

# Paint means either Color or Color+Transparency
abstract AbstractPaint{T, N} <: FixedVector{T, N}

# AbstractColor means just color, no transparency
abstract AbstractColor{T, N} <: AbstractPaint{T, N}
abstract Color{T} <: AbstractColor{T, 3}
abstract AbstractRGB{T} <: Color{T}

# Types with transparency
abstract Transparent{T,N} <: AbstractPaint{T,N}
# Because storage order matters for some applications, we have both
# "AlphaColor" and "ColorAlpha"
abstract AbstractAlphaColor{T,N} <: Transparent{T,N} # storage order alpha-color
abstract AbstractColorAlpha{T,N} <: Transparent{T,N} # storage order color-alpha

# Concrete types
immutable RGB{T} <: Color{T}
    r::T
    g::T
    b::T
end

# ...and so forth

immutable AlphaColor{C<:Color,T} <: AbstractAlphaColor{T,4}
    alpha::T
    c::C   # C really has to be Color{T} (use inner constructor here)
end

immutable ColorAlpha{C<:Color,T} <: AbstractAlphaColor{T,4}
    c::C
    alpha::T
end

# Grayscale
immutable Gray{T} <: AbstractColor{T,1}
    val::T
end

immutable AlphaGray{G,T} <: Transparent{T,2}
    alpha::T
    c::G
end

immutable GrayAlpha{G,T} <: Transparent{T,2}
    c::G
    alpha::T
end
JeffBezanson commented 9 years ago

The plan sounds good. Renaming Color.jl to Colors.jl is probably a good idea in any case.

I'd like to better understand why a color should be considered a FixedVector. Sure, colors form a vector space, but then again lots of things do. Often they are treated like scalar quantities within a program (e.g. like complex numbers). isa(RGB(), AbstractArray) seems a bit odd to me.

SimonDanisch commented 9 years ago

Oh perfect timing, I was just starting to look into this! Obviously this split wasn't intended! I was mainly waiting for FixedSizeArrays and FileIO to mature, which slowly starts to happen.

@JeffBezanson my main intention is to share a lot of functions. Starting from core functions like length, eltype (defined on the type and instance), over construction, simple conversion, math, to functions defined for arrays of FixedSizeVectors. Its really a lot of functionality which would be nasty to duplicate. isa(RGB(), AbstractArray) seems to be odd to me as well as no one does it this way but I also don't see an appealing reasion why its bad. We can always discuss to bend the type hierarchy a bit to account for this, but I'm not sure who would benefit from it. So far isa(RGB(), AbstractArray) is false, as it got me into an ambigouity hell :D But this should change in the future. I would need to refactor arraymath.jl a bit to make this happen (see JuliaLang/julia#11610). I actually started building a benchmarking suite for array operations in order to get a good baseline before I start messing with Base. The first wave of changes should be relatively straight forward, but I really don't want any speed regressions whatsoever.

SimonDanisch commented 9 years ago

@timholy this looks good to me! My two problems are construction of transparent colors and indexing them... But as I don't have a general problem with their structure, I'm sure we find an elegant solution there.

SimonDanisch commented 9 years ago

@timholy would you mind to post a nice example and requirements of your perfect world to JuliaGeometry/meta#8 ? Like, how you want to ideally interact and represent the different images (sliders, tiles of different images, animations, history of previosly applied operations, look of the gadfly plot, etc). Collecting them in one place helps me getting a better mental picture of the different challenges our visualization pipeline faces.

SimonDanisch commented 9 years ago

@JeffBezanson It's just quite likely that you'll work with Vec's and RGB's at the same time, which makes it even more desirable to have the same interface.

a = [Vec3(1f0) for i=1:10]::Vector{Vec3{Float32}}
b = [rand(Vec{3, Float32} for i=1:10]
a[1][1:2]
map(RGB, b)
RGB([1,2,3])
Vec(Vec(2,3, 4), 1) + Vec(1,2,3,4)
a[1:end, 1] # -> [x,x,x,x, ...]
mean(a)
...

These are all functions I would like to see for all scalar like types. By the way, I'm using this for my benchmark package as well, which is quite nice for mean, max etc.

immutable BenchData{T}  <: FixedVector{5, T}
    time_ns         ::T
    gc_bytes        ::T
    gc_time_ns      ::T
    gc_num_pause    ::T
    gc_full_sweep   ::T
end
a = zeros(BenchData, N)
for i=1:N
a[i] = @bench expr
end
mean(a) #etc

Its also nice to do k-means and PCA and things like that on e.g. RGB directly, which would be a lot easier if its a subtype of FixedSizeArray (Which would work for arbitrary other data types which inherit from FixedSizeArrays).

SimonDanisch commented 9 years ago 
immutable AlphaColor{T, C<:Color{T}} <: AbstractAlphaColor{T,4}
    alpha::T
    c::C   # C really has to be Color{T} (use inner constructor here)
end

Can we expect this to work at some point via some triangular dispatch alike pattern?

timholy commented 9 years ago

Obviously this split wasn't intended! I was mainly waiting for FixedSizeArrays and FileIO to mature, which slowly starts to happen.

I know it seems daunting, but I think it's better to make core infrastructure reasonably mature before commencing on such a split. Otherwise you're basically asking other people to carry out that maturation for you (see https://github.com/JuliaIO/FileIO.jl/commits/master). And how do you otherwise know it will work?

Starting from core functions like length, eltype (defined on the type and instance), over construction, simple conversion, math, to functions defined for arrays of FixedSizeVectors. Its really a lot of functionality which would be nasty to duplicate.

I see the point, but for Images I do a lot of that already: https://github.com/timholy/Images.jl/blob/cb9c0811cef911ff590fd58dc4eab3bc6d17274f/src/colortypes.jl#L225-L577. Is that really the only reason you use FixedSizeArrays? I thought there might be some magic GPU reason :smile:.

As an example of what can make this kind of thing hard, what you'd probably like is something like this:

immutable ColorAlpha{T,N,C{T,N}} <: FixedVector{T,N+1}
    c::Color{T,N}
    alpha::T
end

but the N+1 part of that is something we may never have.

Can we expect this to work at some point via some triangular dispatch alike pattern?

I think it's on the slate in https://github.com/JuliaLang/julia/issues/8974. The code in Images is looking forward to a nice haircut once that merges.

@timholy would you mind to post a nice example and requirements of your perfect world to JuliaGeometry/meta#8 ?

I think "vision about how the whole visualization pipeline should work" is a little too broad to be relevant here.

Its also nice to do k-means and PCA and things like that on e.g. RGB directly, which would be a lot easier if its a subtype of FixedSizeArray (Which would work for arbitrary other data types which inherit from FixedSizeArrays).

reinterpret is always your friend. Is there stuff you can't do that way? This is indeed an example of code that's looking forward to a haircut, but see https://github.com/timholy/Images.jl/blob/cb9c0811cef911ff590fd58dc4eab3bc6d17274f/src/core.jl#L127-L202.

On balance, so far I now lean away from the notion from making color types subtypes of FixedVector. Can you live with that?

SimonDanisch commented 9 years ago

Is that really the only reason you use FixedSizeArrays? I thought there might be some magic GPU reason.

Well the magic gpu reason is that it gives me an interface that I can program against, of which I can be sure that the GPU can handle it. Otherwise I would need to put Any everywhere and do a few checks in every involved function. What do you mean by "really only"... 350 lines of code duplication with a slightly different interface means quite a lot to me. What I want to achieve is, that the user can be like "This worked for Vec, maybe it works for RGB exactly the same", et voila, it just works... This always yields extraordinary user experience, instead of frustrating people by having slightly different semantics for similar operations. Also if we start to get more and more into optimizing the code, code duplication means we need to either maintain two high performance code bases, or one party will always be slower than the other.

Reinterpret is nice and all, but you loose type information, which you have to bring back at some point (e.g. when visualizing). I mean its not the worst, but why bother if you can easily just keep it? On top of that, PCA is best implemented with some notion of a FixedSizeVector, so I think it fits well together.

I think "vision about how the whole visualization pipeline should work" is a little too broad to be relevant here.

Definitely not relevant here, but the vision is made up of a lot of small parts which need to work together and i'm interested in how you would like to work with this.

SimonDanisch commented 9 years ago

Also, if I remember the results of my Images.jl load time benchmarks correctly, these https://github.com/timholy/Images.jl/blob/cb9c0811cef911ff590fd58dc4eab3bc6d17274f/src/colortypes.jl#L225-L577 lines of code where accountable for a big fraction of the load time. This code duplication is one of my main arguments for using FixedVector for color types. I really don't see how having this in Images is an argument against using fixed vectors! By the way, LLVM seems really smart about FixedSizeArrays. All the functions defined on them only get compiled one time for types with the same underlying LLVM type. So quite a lot more efficient codegen, compared to having these functions defined for every single color type.

SimonDanisch commented 9 years ago

Also, I started using tuples, which at some point will use llvm's vector type, giving us better chances for simd optimizations. Which is a big plus for Images, I suppose. https://github.com/SimonDanisch/FixedSizeArrays.jl/pull/10 Though, I'm not sure if this is a particular nice view:

immutable RGB{T}
_::NTuple{3, T}
end

When we can overwrite getfield it might be bearable though. Or there was the discussion of just representing homegenous types as llvm vectors, so we might not be forced to use tuples.

In the meantime I introduced a type called FixedVectorNoTuple for colors. I think I can slowly get rid of it though, since I started handling construction and indexing slightly different, to be more independant of the underlying type.

timholy commented 9 years ago

Well the magic gpu reason is that it gives me an interface that I can program against, of which I can be sure that the GPU can handle it. Otherwise I would need to put Any everywhere and do a few checks in every involved function.

You can have the same function defined for Vec and RGB.

This code duplication is one of my main arguments for using FixedVector for color types. I really don't see how having this in Images is an argument against using fixed vectors!

We could have a ColorVectorSpace.jl package for people who want such operations defined on color values. For people who prefer to think about them as "scalars," those operations don't entirely make sense anyway. (See also https://github.com/JuliaLang/julia/issues/8319#issuecomment-56451895, which is 100% correct from a mathematical and colorimetry perspective, but often impractical for operations that many people care about). So a separate package is a good idea.

I could be persuaded that it might be workable to use FixedVectors for the color types. But clearly the priority needs to be FixedArray <: AbstractArray, which I only now realize isn't how it works currently. Until that's done, it seems crazy to make Color <: FixedArray---otherwise you might get yourself into a situation where you'd like to set FixedArray <: AbstractArray but can't because of color-related ambiguities.

FixedArrays are so much closer to AbstractArrays than to Color types, there really shouldn't even be any debate about this. So if you want Color <: FixedArray, let's first see what happens when you make the first transition. Until then, let's keep the color types separate.

Also, if I remember the results of my Images.jl load time benchmarks correctly, these https://github.com/timholy/Images.jl/blob/cb9c0811cef911ff590fd58dc4eab3bc6d17274f/src/colortypes.jl#L225-L577 lines of code where accountable for a big fraction of the load time.

Navigate to the images directory in TestImages.jl, then:

julia> @profile imread("earth_apollo17.jpg")
RGB Images.Image with:                                                                                                                                                                                                                                                         
  data: 3000x3002 Array{Color.RGB{FixedPointNumbers.UfixedBase{UInt8,8}},2}                                                                                                                                                                                                    
  properties:                                                                                                                                                                                                                                                                  
    IMcs: sRGB                                                                                                                                                                                                                                                                 
    spatialorder:  x y                                                                                                                                                                                                                                                         
    pixelspacing:  1 1                                                                                                                                                                                                                                                         

julia> Profile.print()
275 task.jl; anonymous; line: 92                                                                                                                                                                                                                                               
 275 REPL.jl; eval_user_input; line: 63                                                                                                                                                                                                                                        
  275 profile.jl; anonymous; line: 16                                                                                                                                                                                                                                          
   275 /home/tim/.julia/v0.4/Images/src/io.jl; imread; line: 139                                                                                                                                                                                                               
    178 /home/tim/.julia/v0.4/Images/src/io.jl; imread; line: 263                                                                                                                                                                                                              
     178 /home/tim/.julia/v0.4/Images/src/ioformats/libmagickwand.jl; readimage; line: 226                                                                                                                                                                                     
    47  /home/tim/.julia/v0.4/Images/src/io.jl; imread; line: 291                                                                                                                                                                                                              
    50  /home/tim/.julia/v0.4/Images/src/io.jl; imread; line: 322                                                                                                                                                                                                              
     50 /home/tim/.julia/v0.4/Images/src/ioformats/libmagickwand.jl; exportimagepixels!; line: 180

Those two lines are both ccalls to ImageMagick.

By the way, LLVM seems really smart about FixedSizeArrays. All the functions defined on them only get compiled one time for types with the same underlying LLVM type. So quite a lot more efficient codegen, compared to having these functions defined for every single color type.

The same should apply for any dedicated color type hierarchy that looks a lot like FixedVector. You may have to compile two versions of functions (one for Vec3 and one for ColorVector3), but specific color subtypes should be no different from how you have them now.

lobingera commented 9 years ago

Hello colleagues,

i have to say, i'm not very deep into the differentiation between Arrays, Vectors, FixedArray etc. so i cannot contribute on this. I like the up-thread mention of color as scalar value vs. a vector in a color space. I also see, that there is a color representation as tuple of components (real valued, until further notice) starting at 3 with RGB/HSV but with the introduction of spot colors (maybe from a palette like PANTONEtm) does give us an N-space of components. At the same time there are the machine formats in bits/bytes/words and it's always tempting to deal with them in memory only. An emphasis should be spend here to support conversion.

If this is done like doing a new packages Colors.jl replacing (all) the available interfaces in Colors.jl and integrate (all) color conversions i fear again the 'Graphics' situation where packages slightly went out of sync, so changes like this should have a roadmap and some process to catch all dependencies.

SimonDanisch commented 9 years ago

Navigate to the images directory in TestImages.jl, then:

I meant the package load time ;) Loading only color_types.jl: ~6s (3.5 seconds for loading Color etc) Loading Images.jl: ~ 12s

The same should apply for any dedicated color type hierarchy that looks a lot like FixedVector.

That's for sure, but that means rewriting the color types to essentially look like what I did in FixedSizeArrays. My try at refactoring color_types.jl was the primer for me to implement FixedSizeArrays.

@jiahao's comment is definitely noteworthy. How about going with ColorVectorSpace.jl, which relies on Colors and FixedSizeArrays and just overloads the functions that deviate?! So without it, Colors just uses generic +,*, etc and with it, it uses versions specialized to the color space?

But clearly the priority needs to be FixedArray <: AbstractArray, which I only now realize isn't how it works currently.

Yes, this is really unfortunate. It's sad how hard it is to work on abstract interfaces in Julia. I hope triangular dispatch will make this quite a bit easier. I think its a fair concern and I can live with the AbstractPaint interface in the meantime. I would just be very sad to drop FixedSizeArrays for colors entirely, as I'm a big fan of having this rich interface defined for many types. We can try to just implement all the functions in all the packages that use similar types, but that just sounds nightmarish.

I like the up-thread mention of color as scalar value vs. a vector in a color space.

@lobingera Can you explain this a bit more? I'm not really sure, why we should pretend that its a scalar, while we share 90% of the functions with FixedSizeVectors.

timholy commented 9 years ago

I meant the package load time ;) Loading only color_types.jl: ~6s (3.5 seconds for loading Color etc) Loading Images.jl: ~ 12s

Ah, yes, formerly that was really important, and the main reason I had basically stopped extending Images. It's not an issue now, though:

julia> tic(); using Images; toc()
elapsed time: 0.928678548 seconds
0.928678548

(Obviously, using precompilation. EDIT: That's without have Color already loaded. If you've already said using Color, then it's 0.42 seconds.)

timholy commented 9 years ago

I think its a fair concern and I can live with the AbstractPaint interface in the meantime.

OK, that's great! I'll put that effort into ColorTypes.jl so you get to have the more granular organization you want. If your FixedVector gets to the point of being able to serve as the abstract super-type, we can reopen the discussion.

SimonDanisch commented 9 years ago

Much less of an issue I would say ;) We still shouldn't stress codegen like that. After all, even with precompilation, this means including images into my packages nearly doubles load time.

SimonDanisch commented 9 years ago

Great! I'll make sure to help wherever needed. Thanks a lot for this Tim!

timholy commented 9 years ago

We still shouldn't stress codegen like that.

With precompilation, IIUC all those @eval and subtype statements are no longer "there"---they've been executed, and the results (which are straightforward function definitions) are cached.

SimonDanisch commented 9 years ago

Sure, but compilation takes longer and the cache will be bigger, I suppose. Anyways, I do agree that it's much closer to 0 now and I think we found a solution for now :)

lobingera commented 9 years ago

@SimonDanisch Sometimes you choose color (or access color) just by name, sometimes by an index into a palette, sometimes by a real value in a 1D colorspace (continuos colorpalette e.g. jet in matlab). So the components do not show up (on the interface).

timothyrenner commented 9 years ago

I don't feel like I have enough understanding to really contribute to the discussion, but @timholy I'd definitely accept the PR once a consensus has been reached. I'm also be happy to make the switch for ColorBrewer myself to save you a (probably very very small) bit of time if that would make things easier.

timholy commented 9 years ago

OK, @SimonDanisch and I have been hard at work, and we have a lot of progress to report. There are three new packages, all hosted at JuliaGraphics:

I'd like to open the floor for comments on the naming, design, and implementation of those packages. Given the disruptive nature of this change, I should justify why I think it's worth switching:

This was a pretty huge pile of work. I hope folks like it!

One important detail: this will basically cause interoperability-chaos if some packages switch and others do not, or if there are long lags in switching. So first, I want to give folks a couple of days to look the new packages over and make comments; we'll modify these packages accordingly. Once that's done, it's time to transition to using them. If you want to transition your packages yourself, that's great, otherwise I'll submit pull requests for many of them. But, I propose that we hold off merging until an appointed "C-day" (Colors-day) and try to synchronize this transition across packages as well as possible.

StefanKarpinski commented 9 years ago

This is a great plan and a huge amount of work. Thank you both for undertaking it.

One minor suggestion: how about Tint instead of Transparent?

eschnett commented 9 years ago

I see that HSV has h in the range [0,360]. Is that indeed the case? Isn't this a value in degrees, so the interval should rather be [0,360), i.e. with an open-ended interval? Is this a documentation issue only?

stevengj commented 9 years ago

Transparent is not a noun. Tint in standard English is just a "light color", so it seems unnecessarily obscure to me. Maybe TransparentColor?

stevengj commented 9 years ago

Why aren't ColorAlpha and AlphaColor concrete types ColorAlpha{T<:Color} and AlphaColor{T<:Color}, since it seems like for every XXX there are AXXX and XXXA transparent versions?

StefanKarpinski commented 9 years ago

According to this article, "tint" and "shade" already have technical meanings in color theory, so it's probably best not to use that. TransparentColor seems pretty safe.

StefanKarpinski commented 9 years ago

Are the ranges on things like HSV standard? I find the fact that it's measured in degrees jarring. A unit range or radian would both feel much more natural.

StefanKarpinski commented 9 years ago

It seems that Matlab hue value range from 0 to 1, which seems saner to me:

http://www.mathworks.com/help/matlab/ref/hsv2rgb.html

timholy commented 9 years ago

Transparent is an adjective ... how about TransparentColor

Yeah, I agree that's a problem. But TransparentColor has one big drawback: Color is being used to mean a 3-component color, and Transparent doesn't make any assumptions about how many components there are: it's a supertype for both color-alpha and gray-alpha paints. How about Dye? Tinge? Composite? Blendable? Or my favorite, Translucon (yes, I made that up).

Why aren't ColorAlpha and AlphaColor concrete types ColorAlpha{T<:Color} and AlphaColor{T<:Color}, since it seems like for every XXX there are AXXX and XXXA transparent versions?

Because parametric typealiases still have nasty issues:

julia> abstract Kolor{T}

julia> immutable KolorObj{T} <: Kolor{T}
           val::T
       end

julia> immutable KolorAlpha{K<:Kolor,T}
           k::K
           alpha::T
       end

julia> KolorAlpha{K<:Kolor,T}(k::K, alpha::T) = KolorAlpha{K,T}(k,alpha)
KolorAlpha{K<:Kolor{T},T}

julia> KolorAlpha(KolorObj(0.8),0.65)
KolorAlpha{KolorObj{Float64},Float64}(KolorObj{Float64}(0.8),0.65)

# OK, now let's try to make it easier on our users

julia> typealias KObjA{T} KolorAlpha{KolorObj{T}, T}
KolorAlpha{KolorObj{T},T}

julia> KObjA{T}(k::T, alpha::T) = KolorAlpha(KolorObj(k), alpha)
ERROR: cannot define function KObjA; it already has a value

# OK, try it like this (this only works on 0.4, and we want to support 0.3):
julia> Base.call{T}(::Type{KObjA{T}}, k::T, alpha::T) = KolorAlpha(KolorObj(k), alpha)
call (generic function with 1028 methods)

# Now let's use it
julia> KObjA(0.8, 0.65)
ERROR: MethodError: `convert` has no method matching convert(::Type{KolorAlpha{KolorObj{T},T}}, ::Float64, ::Float64)
This may have arisen from a call to the constructor KolorAlpha{KolorObj{T},T}(...),
since type constructors fall back to convert methods.
Closest candidates are:
  call{T}(::Type{T}, ::Any)
  convert{T}(::Type{T}, ::T)
 in call at essentials.jl:57

julia> KObjA{Float64}(0.8, 0.65)
KolorAlpha{KolorObj{Float64},Float64}(KolorObj{Float64}(0.8),0.65)

Are the ranges on things like HSV standard? I find the fact that it's measured in degrees jarring. A unit range or radian would both feel much more natural.

I'll let the colorimetry folks weigh in on that one---I just copied what's in this repo. If we're going to change it, now's the time. (I note the wikipedia page uses degees: https://en.wikipedia.org/wiki/HSL_and_HSV)

kmsquire commented 9 years ago

Thanks for all of your work, Tim and Simon!

I like TransparentColor for Transparent (call it what it is!).

[0-360) for hue in HSV seems to be pretty standard.

lobingera commented 9 years ago

+1 for TransparentColor and still the question: Why not ColorN (which is afair the term used in PDF) for colormodels with more than 3 components on the left hand side of the hierarchy?

lobingera commented 9 years ago

... i just looked it up: and it's called DeviceN. http://wwwimages.adobe.com/content/dam/Adobe/en/devnet/pdf/pdfs/PDF32000_2008.pdf and there 8.6 Colorspace and maybe for inspiration figures 20/21

Which raises another question: Where actually would CMYK be?

lobingera commented 9 years ago

And to clarify: I'm in full appreciation of the work that was already done and thank Tim+Simon for their achievements!

timholy commented 9 years ago

Why not ColorN/DeviceN...and where would CMYK be?

Because N is a parameter, so it would be redundant to have it in the name. EDIT: CMYK would be declared as a subtype of AbstractColor{T,4}.

I'm leaning towards TransparentColor despite the issues, but will wait to see if anyone else comes up with a better suggestion. I did realize that one way to symmetrize this scheme would be to change AbstractColor to OpaqueColor. I kinda like that.

timholy commented 9 years ago

Oh, and @lobingera, I appreciate the input! Exactly what I am looking for.

SimonDanisch commented 9 years ago

+1 to OpaqueColor! It encapsulates much better what it stands for in the hierarchy.

lobingera commented 9 years ago

+1 to OpaqueColor

stevengj commented 9 years ago

Having both Color and AbstractColor/OpaqueColor as abstract types confuses me.

timholy commented 9 years ago

What do you suggest, then?

timholy commented 9 years ago

How about Union(Color3{T}, Color4{T}, Color1{T}) <: OpaqueColor{T,N} <: Paint{T,N}? EDIT: and nothing named Color.

stevengj commented 9 years ago

That sounds clearer.

timholy commented 9 years ago

Might be best to make them typealiases for, e.g., OpaqueColor{T,3} rather than their own abstract types.

Also, would people like the inheritance diagram better (or worse) if I included the type parameters?

stevengj commented 9 years ago

@timholy, my preferences would be to just drop the typealiases. It's not that hard to type OpaqueColor{T,3}.

I think the diagram would be clearer with parameters.

stevengj commented 9 years ago

I would also tend to use AbstractPaint rather than Paint, which sounds a little concrete.

Actually, make it AbstractColor, and have OpaqueColor and TransparentColor be subtypes. A lot better than switching to a completely different noun.

StefanKarpinski commented 9 years ago

I think I agree with @stevengj's general proposal.

timholy commented 9 years ago

I'm fine with that. I do like the brevity of Paint, but it's more important to make the naming scheme consistent and understandable. Barring further input, I'll be adopting the SGJ naming scheme.

(@stevengj, thanks for some great suggestions!)

StefanKarpinski commented 9 years ago

Does it make sense to mock up a new version of the type hierarchy before changing all the code?

StefanKarpinski commented 9 years ago

Something about the fact that Color is a different point in the hierarchy than AbstractColor bothers me. I think it's the fact that "color" is being used both a general term that encompasses grayscale and polychromatic colors and as a more specific term in opposition to monochrome.

stevengj commented 9 years ago

@StefanKarpinski, I thought the proposal was to eliminate Color from the hierarchy entirely?