-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add tests for conversions #48
Conversation
Would you mind pointing out where the specification says that a* and b* are within +-100? I can't seem to find it... |
let kappa: T = flt(841.0 / 108.0); | ||
let delta: T = flt(16.0 / 116.0); | ||
|
||
let convert = |c: T| -> T { if c > epsilon { c.powf(T::one() / flt(3.0)) } else { (kappa * c ) + delta } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be written as a regular function. It can still live inside from_xyz
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!!
Does it do this even when converting from hsl > rgb > xyz?
I checked it in their online converter and chroma is close to 0, so I suppose it's just a color perception quirk. http://www.easyrgb.com/index.php?X=CALC gives the same result. |
I don't know of a range, but whenever I have seen Lab values scaled, it has always been divide by 100. So just like Xyz, a and b values can be above 1.0
Yes. I linearized the rgb and verified the Linear Rgb <-> Xyz conversions. The Hsl conversions match with the Rgb values (non linear). All other online converters do that.
http://www.brucelindbloom.com/index.html?ColorCalculator.html. Shows values close to 0. I checked atan2 in with what we are seeing and it matches up even in javascript. I am not sure where 296 is from. |
I have only seen it with the ranges L: 0 - 100, a: -127 - 128, b: -127 - 128, to adapt them to
Hmm, ok. I guess we can't do much about that for the moment, except jumping through hoops to force things into the correct format.
I changed it from D50 to D65, and that gave me 270. I suspect rounding and/or accuracy errors, but I don't know. Could be a different XYZ -> Lab formula. |
Actually I don't think I have ever seen Lab values scaled in anywhere. Not sure why I assumed they are scaled by 100. I can't find any source now. Everything I have seen so far shows the unscaled values. The official stadard for Lab color space is ISO 11664-4:2008(E)/CIE S 014-4/E:2007. I don't have access to this. Not sure if it has any information on this.
It's an issue with the Lab to Lch direct conversion. The formula used atan2(b,a). I suspect its due to the atan2 implementation differences in the languages. |
I wonder why ISO standards aren't, at least, available in digital form. Feels somewhat counterproductive, but I guess that's how it's funded. Anyway, my university library didn't include ISO in their magical proxy, so I'm also prevented from accessing it. Wikipedia says that "The scaling and limits of the a* and b* axes will depend on the specific implementation of Lab color, as described below, but they often run in the range of ±100 or −128 to +127" (I got the 127/128 ends mixed up) , but there's unfortunately no source for that, and then proceeds to give an implementation for the −128 to +127 variant.
The a and b values are slightly different between the calculators. It's a microscopic difference, but it's enough to make the hue different. |
For Lab -> Lch, there is only an issue with values close to zero. For some of the other hues values I looked at, the conversion was correct. So we know the formula is good. It's just that we can't properly test it now. We can leave it as is. |
For lab, there are 3 choices:
This is my preference.
I don't see the benefit in either of these 2 choices. |
As long as it covers enough to catch mistakes. Lab -> Lch is straight forward enough, so I don't expect it to be much of a problem.
The benefits of rescaling them to be percentage based would be to make them more consistent with the rest of the library, and to make multiplication easier to handle (no post-multiplication scaling is needed). It's the same arguments as before. It's unfortunate that the formulas seem to favor the byte format over everything else, while this library is more concerned by humans and math. In any case, this change doesn't belong here, if it's to be made at all. It's not a bug fix, since it's intended behavior. |
Right now lab a,b values are assumed scaled by 100. Everything works. I am not sure what to do here. If we assume a,b scaled by 128. Then the lab -> chroma and lab -> xyz formula's and the tests need to be reworked to account for this. If we are eventually move away from this anyway, I'd rather not make this change. |
Something has clearly been confused. The original conversion was based on the -128 - 127 scale, and then rescaled by 1/128. The reverse scaling (a * 128 and b * 128) was then performed when going from Lab to XYZ. You are currently doing the same with 100, so what's the difference?
The scale is either preserved as it was intended to be, or this PR gets to wait until it's time for 0.3.0. I will only merge breaking bug fixes for patch level releases, and not when intended behavior has been changed, no matter how much anyone of us wants it. It's for the sake of the users. |
Yeah I thought there was a scaling issue in Xyz -> Lab or Lab -> Xyz conversion as I mentioned in the changes. I just checked again there is not issue there. There is a problem currently with the Lab -> Lch conversion them. Shouldn't the Lab values be scaled for the chroma conversion? So currently all other conversion seem ok (apart from the div by zero for Hsl Hsv)
That is basically my question. What is the path forward? I don't want to change things now and then change them back again. It'll just be unnecessary work. If we are making the change I'd rather wait. |
Part of what's bothering me is that I have spent way too much time over past couple of days making sure the scaling is right. Scaling the test input colors and then again making sure the scaling is correct in all the formula's etc
We are not consistent currently. Hues are 0-360, Xyz > 1, Lch Chroma is > 1, Lab -1 to 1. Yeah maybe multiplication is easier, but we are moving the problem elsewhere into the conversions. |
Keeping it as it is will keep the max value of chroma within 1 - sqrt(2). It will just turn a Cartesian plane into polar plane with the same scale.
Hues are measured in degrees, which is a completely different unit, and little else makes sense. XYZ is special because it's more physically based than the rest, but it's actually normalized to keep Y within 0-1. Chroma is the radius of a circle inside a square. A circle can't fit inside a square and still fill it, so the max limit of Lch chroma will always depend on the hue. a and b in Lab are centered around gray (0, 0) and represents the amount of red and blue, so saying 100% or -100% blue makes more sense to me than 127 blue or 1.27 blue. Being able to multiply things in a sane way is more of a bonus in the Lab case. The fact that this library doesn't try to fit things into single bytes gives it the opportunity to pick types and units that fit the parameters, at the potential cost of strangeness. The question is how much strangeness is too much. Maybe it's best to stick with the exact algorithm and format from the specification, after all. Maybe Lab and Lch isn't normally used in a way that needs the same behavior as RGB tends to do. I don't know, so I'm testing those limits a bit by doing things like this.
I understand, and I do really appreciate all the effort and time you have spend on this, and this is not me being ungrateful, even if I'm not saying "thank you" as often as I should. Where would this be without all of your questioning and contributions? I do know that the more time one spends on something, the worse is it when someone else comes around and demands them to be changed, not to mention rejects them (that's why the Rust team has their RFC process). These tests are incredible, and I'm sorry for starting off by questioning why you changed the scale, without saying that first. I will work harder on my constructive feedback. The sad part (for me as well) is that I have to think about all the consequences for the library. What happens if I would just merge a PR and release it? How will it affect the users? I'm sure you understand those parts, so I won't go on about semver and incremental development, and I hope you also understand that it's the reason why I'm always questioning large and/or sudden changes. I'm not questioning them because I don't like change, but because I want to make sure it's a good change and that it won't come as an unpleasant surprise for the users. People tend to not like surprises when it comes to these things, for some reason, and predicting their behavior is especially hard now when few are using the library in the open. Now, my preferred constructive feedback procedure tells me to not criticize anything without suggesting an alternative way forwards, so, to be able to do that, I'm wondering how much more effort it would be to change the scaling factor back to 128 for now. I know that it affects the unit tests in the lab module, as well as the conversion to and from XYZ. How does it affect the new tests? Are the expected values pre-scaled? |
You may have guessed already, but I'll clarify my plan anyway. I would really like to merge these tests in an non-breaking way (except for the bug fixes), so it would be fantastic if that's possible. That's why I'm asking about how large the impact will be. I have also been thinking some more about the rescaling stuff and the fact that not even CIE suggests any other definition of Lab than the wonky byte format ranges, so the strangeness price may be too high to pay, after all. Some sources (like Wikipedia) suggests that the ranges of some implementations are +-100, but they won't provide any sources or algorithms to show it, and it's almost impossible to find any other algorithms, so it's not as implementation dependent as I first thought. I have more or less changed my mind and decided to say yes to going with the flow. What we would be changing to then would, in my opinion, be no rescaling at all. Not even the L component. Also, don't worry about the extra work. You are not the only one here, even if I have been quite busy this past week. :) The library hasn't stabilized yet, so there is always some wiggle room for things like this, but it will be harder in the future. That's why I'm thinking that we can change to the common variant now and see how it works out. We can then change again if it turns out to be too impractical for what it's used for, but we will at least have the common variant if it turns out to not matter. What do you think about that? |
I tried with the rescaled Lab values, but the tests failed. I did not debug the errors. From the information above most likely due to Lch chroma values. I think I scaled them by 100 too which is wrong. So the only major change is the fixes for hsl hsv conversions. The lab conversion formula is changed but I don't think the existing formula's are wrong, I have seen those also being used. May be just make a PR with just those Hsl, Hsl changes for now? We can add the tests later? |
Yes the expected chroma will (fortunately only) have to be scaled by 1/128, as well. And L by 1/100 in both cases. I think the existing formula for Lab is completely correct. It's only the scale. Do you think that is doable or are the tests too rigid? The changes for Hsl and Hsv can stay if this is mergeable, since they are bug fixes. They were never specified to behave strangely. |
I went ahead and rescaled lab to 128. All tests work now. |
That's great, but I'm still a bit confused when it comes to the numbers in the tables. Are any of those the "correct" unscaled (not even by 100) values? |
No, but since l is divided by 100, I kept a and b also divided by 100. |
Ok, so changing to unscaled should be trivial, right? Just multiplying them by 100 again? |
Any thing else needed on this? |
I'll check it more thoroughly in a moment, but I think the test results speaks for themselves... What was the deal with the ignored tests, again? Were those affected by RGB being assumed to be non-linear? Maybe we should add an issue for that, unless it can be fixed. |
@@ -60,7 +60,7 @@ impl<T: Float> Alpha<Yxy<T>, T> { | |||
|
|||
impl<T: Float> FromColor<T> for Yxy<T> { | |||
fn from_xyz(xyz: Xyz<T>) -> Self { | |||
let mut yxy = Yxy{ luma: xyz.y, ..Default::default() }; | |||
let mut yxy = Yxy{ x: T::zero(), y: T::zero(), luma: xyz.y }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have to ask; is this to pass the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well sort of. I am not sure what the right values here are. If you don't make them 0's there will be a big difference between XYZ(0, 0, 0) and XYZ(0, 0, 0.0001) converted Yxy , that's one of the reasons I made them zero's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is XYZ(0, 0, 0.0001) even a valid value? I had the impression that it's basically black if Y is 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to say when Y = 0.0001
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok. Hmm... The Y=0 are is so strange, since it's mathematically possible to have "red, but no energy", but it makes no sense otherwise. We may just as well give in to the math and see where it takes us.
assert_color_eq!(src.lab, tgt.lab, [l,a,b]); | ||
assert_color_eq!(src.lch, tgt.lch, [l,chroma]); | ||
// removed check due to issue with color mine huw conversion issues. | ||
// assert_color_hue_eq!(src.lch, tgt.lch, [hue]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we activate this for colors where chroma is over a certain limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just tested this but yxy -> lch is failing with check for difference of 0.1 and passes with difference of 4.0. The lab -> lch tests for red, blue and green pass
One failing test is for pink (from_yxy) where the difference is 3.1348 ( close to pi)
Expected value: Lch { l: 0.8358, chroma: 0.19046874, hue: LabHue(7.82) }
Calculated value: Lch { l: 0.8358424, chroma: 0.19743656, hue: LabHue(10.954128) }
the lab values are off by a: 0.005 and b: 0.012.
Alright, I have read through it and commented on some points, but it's otherwise great 😄 This is ready to be merged as soon as those has been addressed, and some kind of action regarding the ignored tests had been taken. |
Again the full tests are failing only on yxy -> lab. This time the difference is 6.24 ( almost 2 pi) degrees for hue.
removing the lch hue check passes all the tests, even the full tests. I have a feeling this is again due to the atan2 issues |
I can't see how from_xyz tests pass and from_yxy don't pass. The formula's seem ok |
Something is not right, here. a in Lab differs by 0.012, which seems like quite a bit, and Lch chroma differs by 0.01. The differences in hue is in degrees, so I doubt it's relate to pi, but what do I know? It's still quite some difference. atan2 shouldn't be that inaccurate. Could it be some typo in the reverse conversion? |
Tried to remove atan2 close to zero values from the equation
still only the from_yxy, full and mini fail. rest all pass. |
The Yxy -> XYZ conversion looks fine to me... I tried to feed the values into the calculator over at EasyRGB and it gave the same XYZ as ours. The expected values must be off. |
...but RGB is off by a factor of 10, so I don't know what's up there. |
Possibly due to this? http://www.brucelindbloom.com/LContinuity.html There are a few different formulas for xyz -> lab conversion. Maybe the slight difference yxy -> xyz in the expected and caluculated values at the boundary is causing this? For yxy converions only look at linear rgb. none of the other rgb derived colors match due to gamma correction. |
The lab <-> xyz formula's are incorrect in the Cie 15: 2004. Let me update those. |
Those shouldn't affect the Yxy -> XYZ conversion and cause it to differ by 0.08 and 0.01 for x and z. That's a bit too much for rounding errors. |
I updated the formula's and its still the same. I noticed the xyz and yxy values not matching. That's why I addded the xyz and yxy converison tests from the cie values just to be sure. Thats also why I reduced the tolerance to 0.05. We just want to make sure the value are not completely incorrect and are in the ball park. For now the tests pass with lch hues test disabled. I don't have any other ideas to test. |
Alright, that will do for now. We can tighten it all up in the future. This should show that we are not completely out there. |
Just tell me when you are ready :) |
8d31fab
to
b3ed470
Compare
Done |
Do you want to ignore the full tests? They will run everytime on travis. Thats why I added a mini test to quickly check a few colors. |
No, they are not that slow, so it's alright. Thank you for all of this! @homu r+ |
📌 Commit b3ed470 has been approved by |
⚡ Test exempted - status |
Add tests for conversions Add conversion tests. Test data is from Cie Illuminants data (for Xyz <-> Yxy conversion tests) and colormine.org (more comprehensive conversions). Closes #44. ### Changes - Xyz -> Yxy conversion changed to use x:0 and y:0 as default when Y = 0 from the earlier D65 white point values. Avoids large changes in Yxy for changes in Xyz y value from 0 to 0.0001 - There are a few different formula's for Xyz <-> Lab conversions, each slightly different. Formulas updated to use the standards. - Hsl and Hsv conversions updated to avoid divide by zero issues. ### Notes - Conversion tested for an accuracy of 0.05. This is to validate that the values are close enough. - ColorMine Linearizes the Rgb values before conversions, so rgb, hsl and hsv conversions are separately tested from xyz, yxy, lab and lch. - ColorMine formula for Lch hue seems incorrect. Ex Gray (128,128,128) gives a Lch Hue of 296 when the value is expected to be 0. Also the Lch hue converions are failing only for yxy converions. So ignored the Lch hue comparisons.
Add conversion tests. Test data is from Cie Illuminants data (for Xyz <-> Yxy conversion tests) and colormine.org (more comprehensive conversions).
Closes #44.
Changes
Notes