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

Added code for additional color difference formulae #28

Merged
merged 3 commits into from
Jun 1, 2014
Merged

Added code for additional color difference formulae #28

merged 3 commits into from
Jun 1, 2014

Conversation

glennsweeney
Copy link
Contributor

A whole slew of color difference formulae for those who just can't be satisfied with Delta E 2000. ;)

Some have industry applications; some are antique but still used. I will try to look up sources and include them soon.

You'll notice that colordiff survived untouched; it is just an alias for colordiff_2000 now. See line 84.

BFD has a feature to allow giving a white point for the LAB->XYZ and XYZ->LAB transform that happens internally. I left it undocumented to avoid confusion.

@StefanKarpinski
Copy link
Contributor

Any time I see a bunch of functions with really similar names, I get pretty suspicious that this should be done with dispatch instead. Perhaps by providing color difference types and then having the color difference type as an optional argument to colordiff?

@StefanKarpinski
Copy link
Contributor

Having color difference objects also naturally provides a place for parameters to the difference computation to live, which is another indication that it's a better way to express this.

@glennsweeney
Copy link
Contributor Author

Ugh. I'm still a C programmer at heart, sorry. I'll see what I can do for that; thanks for the input.

Any suggestions on what the objects could look like to be more coherent with the Julia mentality?

@StefanKarpinski
Copy link
Contributor

No worries – C is the desert island programming language, after all :-)

They should be something like this:

abstract ColorMetric

immutable CIEDE2000 <: ColorMetric end

immutable DeltaE94 <: ColorMetric
  kl::Int
  kc::Int
  kh::Int
  DeltaE94(; kl::Integer=1, kc::Integer=1, kh::Integer=1) = new(kl, kc, kh)
end

immutable JPC79 <: ColorMetric
...

Later you would do something like this:

colordiff(ai::ColorValue, bi::ColorValue, m::ColorMetric) = colordiff(ai, bi, CIEDE2000())

function colordiff(ai::ColorValue, bi::ColorValue, m::CIEDE2000)
  ...
end

function colordiff(ai::ColorValue, bi::ColorValue, m::DeltaE94)
  # use m.kl, m.kc, m.kh values as parameters
  ...
end

Those parameter names could probably be better since they're now externally exposed.

@StefanKarpinski
Copy link
Contributor

Any progress? Does that approach make sense?

@glennsweeney
Copy link
Contributor Author

Yes, it does make sense! Sorry, it's approaching finals week - things are gonna move at a snail's pace for me for a bit. I'll try to get a look at this and put the changes in soon as I can.

@StefanKarpinski
Copy link
Contributor

No worries, just thought I'd ask.

@glennsweeney
Copy link
Contributor Author

Here's an updated version. let me know what you think. I also took the liberty of deleting a .swp file that had snuck into the repository with 0d7e39f. Left it as a separate commit for transparency.

@StefanKarpinski
Copy link
Contributor

Looks good to me but @dcjones should take a look and merge if it's good.

dcjones added a commit that referenced this pull request Jun 1, 2014
Added code for additional color difference formulae
@dcjones dcjones merged commit 9ed73bd into JuliaAttic:master Jun 1, 2014
@dcjones
Copy link
Contributor

dcjones commented Jun 1, 2014

Nice work @glenn-sweeney.

@dcjones
Copy link
Contributor

dcjones commented Jun 1, 2014

The one issue I noticed is this:

julia> colordiff(color("red"), color("blue"), DE_BFD())
ERROR: no method convert(Type{XYZ}, RGB, XYZ)
 in colordiff at /Users/dcjones/.julia/v0.3/Color/src/differences.jl:298

That's just exposing an existing issue though. We have a white point parameter when converting from e.g. LAB to XYZ, but not from RGB to XYZ. So if it makes sense, we should add the same parameter to every convert(::Type{XYZ}, ...) function.

@glennsweeney
Copy link
Contributor Author

I've been noticing some problems as well; for instance, there is no
conversion with a white point from XYZ to LCHab or similar. If we want
to provide this support, it'll just mean adding white point control through
the entire conversion function set. Special care will need to be taken with
examples like RGB - right now we only support sRGB, which uses the D65
white point only.

On Sun, Jun 1, 2014 at 12:29 PM, Daniel Jones notifications@github.com
wrote:

The one issue I noticed is this:

julia> colordiff(color("red"), color("blue"), DE_BFD())ERROR: no method convert(Type{XYZ}, RGB, XYZ)
in colordiff at /Users/dcjones/.julia/v0.3/Color/src/differences.jl:298

That's just exposing an existing issue though. We have a white point
parameter when converting from e.g. LAB to XYZ, but not from RGB to XYZ.
So if it makes sense, we should add the same parameter to every convert(::Type{XYZ},
...) function.


Reply to this email directly or view it on GitHub
#28 (comment).

@dcjones
Copy link
Contributor

dcjones commented Jun 1, 2014

Yeah, a more general RGB implementation would be good. I'd be best if we can do that without forcing people to care about the white point, etc, and default to sRGB.

@glennsweeney glennsweeney deleted the color_diff branch June 3, 2014 11:19
@glennsweeney
Copy link
Contributor Author

It might be worth introducing a type like conversionparams or something that can house relevant customized conversion information, such as a white point, and provide it as an optional parameter to any conversion. That way "longer" conversions through multiple steps could have access to necessary custom data. For instance, if you were converting from LMS to LCHab, it could provide white point information to the intermediate XYZ->LAB step.

This allows a bit of "futureproofing" as we accumulate more metadata needed for certain steps, but when not provided, could always revert to defaults. It could also become the backbone of a general RGB transformation; if nothing is specified, assume the sRGB tone function and D65 white point.

Just a thought, if there's a better way to structure this, I'm happy to consider that instead.

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 this pull request may close these issues.

3 participants