search

JuliaAttic / Color.jl

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

Fix RGB to HSL "L" conversion #94

Closed rickhg12hs closed 9 years ago

rickhg12hs commented 9 years ago

Referencing numerous RGB to HSL conversion algorithms (Google search hits, etc.), the min and max values need to be added, not subtracted.

timholy commented 9 years ago

Looks like the tests need to be updated?

rickhg12hs commented 9 years ago

I updated the tests, but need to admit that I only updated the values with the ones returned by Travis when the tests were failing. I'm not qualified to assess the correctness of the tests or test results.

timholy commented 9 years ago

I'm a little suspicious that the tests with "hue" all have values between 0 and 1, but according to the type definition it seems it should be between 0 and 360. @jiahao, these are your tests, can you check this out?

timholy commented 9 years ago

Perhaps useful for cross-checking: http://colormine.org/color-converter

rickhg12hs commented 9 years ago

Some anecdotal evidence from StackOverflow that this fix may be adequate.

timholy commented 9 years ago

I'm going to merge this, but @jiahao, can you please look at those tests again with respect to the issue I raised above?

dcjones commented 9 years ago

I suspect that those tests are correct but we're just testing a very narrow range of hue values.

timholy commented 9 years ago

I'm a little concerned, though, given that prior to this commit we were testing that the algorithm gave incorrect values.

jiahao commented 9 years ago

@timholy sorry, just got back from a few weeks' travel.

The tests I added were only confirming behavior that was present 8 months ago, back when I wanted to make sure that the vector space addition operation was correct. I had asked for help in asserting correctness, but nothing transpired then. So I'm happy that people are working through the tests and checking them.

timholy commented 9 years ago

Thanks, @jiahao. I'm no color-space guru myself, so I hope some of the other folks who know this stuff chime in eventually.