Failing cli-colorspace(12) test - double value equality

[neuron]

Hi,

I am trying to compile Imagemagick 6.9.4-9 on solaris 10. There are 7 tests failing

FAIL: tests/cli-colorspace.tap 12
FAIL: tests/validate-formats-disk.tap 1
FAIL: tests/validate-formats-map.tap 1
FAIL: tests/validate-formats-memory.tap 1
FAIL: tests/wandtest.tap 1
FAIL: Magick++/demo/demos.tap 3
FAIL: Magick++/demo/demos.tap 6

I started to look at the first failure, ignoring the rest for now. The test 12 is about HSL <-> sRGB conversion.
If I run the test by hand, I get "137,80,146" which is not equal to the original value "146,89,80".

It turns out that the problem is in comparing double values in ConvertRGBToHSL

The function says:

Code: Select all

...
  if (c <= 0.0)
    {
      *hue=0.0;
      *saturation=0.0;
      return;
    }
  if (max == (QuantumScale*red))
    {
      *hue=(QuantumScale*green-QuantumScale*blue)/c;
      if ((QuantumScale*green) < (QuantumScale*blue))
        *hue+=6.0;
    }
  else
...

The condition

Code: Select all

max == (QuantumScale*red)

does not match, because the difference between the values is -3.171291e-17.

I have replaced all the eqality comparisons with

Code: Select all

fabs(a-b) < 0.00001

(I would attach patch if I could find how) and the test is now passing. But I'm not sure whether it's generally good approach. Since you do a lot of floating math, I guess you had to tackle such problems in the past. What would you recommend to do to fix the issue?

Thank you
__
Vlad

[neuron]

Ok, I quickly pasted the patch at http://pastebin.com/PHLAj7SF. It's just to show the approach I have taken.

[magick]

Thanks for the problem report. We can reproduce it and will have a patch to fix it in GIT master branch @ https://github.com/ImageMagick/ImageMagick later today. The patch will be available in the beta releases of ImageMagick @ http://www.imagemagick.org/download/beta/ by sometime tomorrow.

[neuron]

Ok, thanks. I'm curious about what the fix will be, as floating point math is evil

[magick]

The fix is as you suggested. Its never been a problem in the past but best practices for floating point is to compare the results to an epsilon value. The patch should be in Github now.

[neuron]

Thank you, works like a charm.