Review Request: KColorSpace::KHCY::KHCY(const & QColor) constructor floating point precision error

[Benoit Jacob]

Guys, maybe I see this a bit late, but the file in SVN still uses
operator== between qreal's and that's a "no go". The general good
solution is Olivier Goffart's suggestion to use qFuzzyCompare although
in this *special* case since you know the values are in [0;1] you can
use a faster test if it's performance critical, see below.

I'll formulate this as a pseudo FAQ:

*** What's the problem with == and != ?

This part was understood in above e-mails: operator== and != do an
exact comparison, but for floating-point numbers one should only ever
use fuzzy comparisons, because very often floating point numbers that
are "morally equal" are not equal in the sense of ==.

*** So why does it work for us in kcolorspaces.cpp

Because a QColor internally represents color channels as 16-bit ints
and qreal has significantly more than 16 bits of precision even on
platforms (ARM) where qreal==float.

But suppose that someday, this code is extended to support 32 bit int
color channels. Then on platforms where qreal==float, you'll start
seeing the bad effects described below. Also, if you reuse this code
in another context, where the components are stored as floating-point
numbers etc.

In a word: even though it happens to work, it's dangerous.

*** But I just want to avoid division by 0 ! So for an expression like
a/(b-c) the check that I want is really b==c, right ?

No, in general, you don't just want to avoid division by 0, you also
want to avoid the formation of arbitrarily large numbers. If you allow
an operation like a/(b-c) to return very large numbers (because b-c is
very close to 0) then there's a risk that, a few operations further
down the road, you'll obtain "inf" values, and then "nan" on
subsequent operations, which will be hard to debug. For that reason,
when you're going to divide by a value, you want to catch "very close
to zero" values, not just "exactly zero".

*** OK, what is the right way to replace == for floating point numbers?

Use qFuzzyCompare. Here's what it does:

static inline bool qFuzzyCompare(double p1, double p2)
{
  return (qAbs(p1 - p2) <= 0.000000000001 * qMin(qAbs(p1), qAbs(p2)));
}

In other words it checks whether the distance between p1 and p2 is
negligible compared to both p1 and p2.

*** Can I use qFuzzyCompare to check if a number is close to zero?

Never use qFuzzyCompare directly to check whether a number is
approximately zero. Doing
  qFuzzyCompare(p,0)
expands to
  return (qAbs(p) <= 0);
which is an exact comparison, it's basically the same as "p == 0" !

There is no universal way to check whether a floating-point number is
near 0, because e.g. double's can be anywhere between 1e+300 and
1e-300 even though they have only 15 digits of precision. So if you
need to check whether a number is close to zero you need to rephrase
that first.

Of course, "b-c == 0" rephrases as "b == c" so you can use qFuzzyCompare(b,c).

In other cases, e.g. "p == 0", you need to rephrase "close to zero" as
"negligible when compared to a certain reference value" where the
"reference value" comes from your context. The test is then (for
doubles):

static inline bool isNegligible(double p, double reference)
{
  return (qAbs(p) <= 0.000000000001 * qAbs(reference));
}

*** What if I know that my numbers are between -1 and 1, or even
between 0 and 1, and i know furthermore that a number can be declared
"negligible" as soon as it is negligible when compared to 1? (Note:
this is the case in kcolorspaces.cpp)

Then indeed there are some simplifications.

qFuzzyCompare can then be reduced to:
return (qAbs(p1 - p2) <= 0.000000000001);

and isNegligible can be reduced to:
return (qAbs(p) <= 0.000000000001);

Just make sure that you understand why this is very peculiar to this
situation and can't be done in general. Suppose that your floating
point numbers are typically of the order of magnitude of 1e+50 (this
is perfectly possible, double's go up to 1e+300). Then this simplified
version of qFuzzyCompare is an exact comparision like operator== !

Cheers,
Benoit (hoping, unrealistically, that there are no other examples of
== between qreals in kdelibs).

2009/6/25 Oswald Buddenhagen <ossi at kde.org>:
> On Wed, Jun 24, 2009 at 10:52:28AM +0200, Hans Meine wrote:
>> On Wednesday 24 June 2009 00:38:31 Oswald Buddenhagen wrote:
>> > On Tue, Jun 23, 2009 at 04:59:12PM -0400, Michael Pyne wrote:
>> > > against 0.0 (which is really the only equality comparison you can do
>> > > on floating point since it is exact and so are its conversions to
>> > > different forms).
>> >
>> > all small integers (i think +/- 2^23 for single precision) can be
>> > represented exactly.
>>
>> Yes, but a value which is != 42 in one moment might become == 42 just one LOC
>> below (due to extra precision truncation).
>>
> yes
>
>> AFAICS, with zero this will not happen, because of the floating point
>> (mantissa truncation will not make it zero).
>>
> sounds somewhat plausible, but i cannot come up with an actual case how
> that might happen ... it's too late today. :}
>
>> > and fwiw, ((a == b) == ((a - b) == 0.0)) in any case.
>>
>> Unfortunately, such a statement could become false if there is an extra
>> statement inserted after the (a == b) which causes only one of the two
>> variables to be put on the stack (and thus truncated).  That sounds unlikely,
>> but may give you a really evil bug when it happens somehow..
>>
> yes, very unlikely, as comparision is just a subtraction which does not
> store the result, i.e., the register set up is exactly the same up to
> the key instruction - barring arbitrary compiler weirdness.
>
> btw, this 80-bit x86 fpu anomaly can be forced away by putting the
> compiler into strict ieee-754 mode (at a non-trivial performance cost,
> unless everything is done with sse anyway).
>
>