How to handle floating point comparison warnings (DO NOT MERGE)

[DonLakeFlyer]

There are quite a few of these. I've looked at most of them and they are legitimate == 0.0 comparisons when the float was directly set to 0.0. One way to handle them is to turn off -Wno-float-equal. Another is using the mechanism in this pull (or something similar). Looking for comments on what direction to head.

[pigeonhunter]

Can we not test for 0.0 with isfinite() and avoid float==float and pragmas?

if ( isfinite(1.0/ax) ) {
    /* go nuts */
}

[LorenzMeier]

I would prefer either the division or (fabsf(val) > FLT_EPSILON). I would think that my proposal is potentially faster at execution time, but I might be off depending on the actual HW implementation.

[DonLakeFlyer]

Some example use cases:

• Initialization of mag values to 0.0. Then test == 0.0 to see if the mag values have been updated.
• param spec says 0.0 signals something. Then test == 0.0 for that something.
• also just a few with math multiplication, then check for 0.0

To me the direct set to 0.0 then check that it hasn’t changed should be isfinite method. But anything involving math at all should be the fabs method. Comments?

Also: How about still using an fp_helpers.h to define exact_zero helper (using isfinite) as well as a compare (using fabs) helper inline routine. I think these can make things more understandable.

On Jun 30, 2014, at 1:06 AM, Lorenz Meier [email protected] wrote:

I would prefer either the division or (fabsf(val) > FLT_EPSILON). I would think that my proposal is potentially faster at execution time, but I might be off depending on the actual HW implementation.

—
Reply to this email directly or view it on GitHub.

[DonLakeFlyer]

Pushed up some more changes with match my proposal above. Still not compete.

[pigeonhunter]

I don't agree with using two methods to implement identical logic. We should just choose one or the other.

Personally, I find the isfinite approach to be more readable, though possibly less performant. Particularly when there are multiple vars to be tested.

The wrapper is a nice convenience if the epsilon approach is chosen.

[DonLakeFlyer]

The current pull state has all of these cleaned up using the following macros from fp_helpers.h:
#define is_exactly_zero_float(f) (!isfinite(1.0f/(f)))
#define is_exactly_zero_double(d) (!isfinite(1.0/(d)))
#define is_equal_float(f1, f2) (fabsf((f1)- (f2)) > FLT_EPSILON)
#define is_equal_double(d1, d2) (fabs((d1)- (d2)) > DBL_EPSILON)
I have not finished code reviewing all of it, so it is not quite ready and may have chosen the wrong one in some cases.

The question is whether the current code paths which look likes this:
float f = 0.0f; // 0.0f used to signal a specific case as opposed to a real math based value
... // Do some stuff, which may set f to something other than 0.0f
if (f == 0.0f) {
// Do something when f was unchanged
}
Should this code use is_equal_float or is_exactly_zero? If you look through the pull you'll see examples of both use cases. I've gone with is_exactly_zero.

You still need is_equal_float/double as well since there is code which is doing non-zero float compares as well. Or doing zero compares which are after mathematical calculations. These case should use an epsilon based comparison.

[px4dev]

FLT_EPSILON sounds like a good idea, but in practice it's not good enough. Neither is the 1/x test for sufficiently small values of x.

See here for a solid discussion of the issue: http://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

In general:

• Never use special floating-point values as flags. Just don't do it. Passing extra arguments to a function is cheap by comparison, and has the added benefit of being much clearer.
• Never perform equality comparisons on floating-point values. Code that assumes it can is fundamentally broken and needs to be fixed.

(yes, these are simplistic generalisations, but as rules of thumb they are a safe starting point)

[DonLakeFlyer]

I don't disagree with any of that. But if you look at the code base you'll find these use cases which I don't think you can get rid of:

• Common Mavlink parameters which are specified as float, but use 0.0 and 1.0 to signal off/on values. You can't change those unless you want to lose compatibility with APM since they are in the common section. Arm/Disarm command is an example.
• Code which is looking for 0.0 in floating point calculations to special case math which would cause a nan calculation.
  I don't think there is any way to get rid of this, so there will need to be some way to silence the warnings for them.

The rest mainly fall under the don't do that rule. This pull has patches for every single one of them in the code base. Although the approach I took may be incorrect it may be a useful place to comment on each as to what should be done about it. If someone wants to point me in the right direction, I'd be happy to make the changes once a direction for each is set. Whether that is, add separate signal flags or what not.

[DonLakeFlyer]

If folks are interested in taking this commenting approach to the discussion I can start the individual comments myself since I've gone through all of these and have decent context for most.

Updating my comment: If you look through the code there about 10-15 real issues here. The rest are unit test code, testing floats. So it's not too crazy to look at these individually.

[px4dev]

Common Mavlink parameters which are specified as float, but use 0.0 and 1.0 to signal off/on values. You can't change those unless you want to lose compatibility with APM since they are in the common section. Arm/Disarm command is an example.

If we're stuck with these in a wire protocol, you can rewrite them as magnitude comparisons vs. 0.5f; I think this is likely to be sufficiently clear, and at the same time ugly enough to encourage a search for a better representation in the future. 8)

Code which is looking for 0.0 in floating point calculations to special case math which would cause a nan calculation. I don't think there is any way to get rid of this, so there will need to be some way to silence the warnings for them.

If the issue here is division-by-zero, there is also the issue of INF results to consider, and comparing against zero isn't going to be sufficient there. In general, unless we expect the calculation to blow up in the normal case, I think we're better off doing the math and then checking the result for finity...

[DonLakeFlyer]

Seems like this one should have some sort of specified delta which is used to determine whether the previous versus new sonar values are different enough to care about. Certainly larger than FLT_EPSILON would seem to be too small to care about.

[LorenzMeier]

Does anyone know the required precision to avoid a division by zero / NaN return? That sounds like the other critical barrier we need to meet.

[Kynos]

I know it ain't exactly pretty but in other contexts I have simply been checking for NaN, negative & positive infinity after division.

[DonLakeFlyer]

That seems like the safest way to do it.

[DonLakeFlyer]

Ok. Thanks everyone. I'm going to take all this feedback and make a new pull and see what everyone thinks again. I'm going to leave this pull open for now as a reference.

[jean-m-cyr]

I see nothing particularly wrong with using float equality with zero as a flag, other than the fact that the compiler endlessly complains about it! All of the methods suggested here cause the compiler to generate extra instructions. One way around the compiler warning that will not trigger the warning nor cause the generation of extra instruction (with the optimize flags we use) follows:

Instead of

if (x == 0.0) ...

use

if (!((x < 0.0) || (x > 0.0))) ...

This would of course need to be incorporated as a macro. ie:

#define floatEquals0(a) (!((a < 0.0)||(a>0.0)))

[LorenzMeier]

@jean-m-cyr The proposal is fair and would be equivalent as you note. However, adding the additional requirement of really offering a div-by-zero check, I'm wondering what a suitably large number would be in float. We will not be able to give a definite answer there, but I would suspect checking for zero is not enough - we probably need something like num < 1e-7 to be on the safe side.

[jean-m-cyr]

I was only referring to the case where 0 is assigned to a float to indicate an uninitialized state (often the case in PX4 code). As in:

float x = 0.0f;
...
if (x == 0.0f) {
x = sensorValue();
}

[px4dev]

Jean,

When passed to a function, this is actually more expensive (move from vector register to GP register, arithmetic) than just stacking an argument to the function.

In general, in-band magic values are bad; we should not use them.

On Jul 3, 2014, at 11:13 AM, Jean Cyr [email protected] wrote:

I was only referring to the case where 0 is assigned to a float to indicate an uninitialized state (often the case in PX4 code). As in:

float x = 0.0f;
...
if (x == 0.0f) {
x = sensorValue();
}

—
Reply to this email directly or view it on GitHub.

[jean-m-cyr]

Only true if the float is not be passed as an argument in the first place,
If you have to pass both the float and another parameter to say whether it
has been initialized or not, then you're no further ahead.

I agree with you on avoiding magic values, but we're way past that in our
use of 0.0 for that purpose. In an ideal world, our builds would generate
NO compiler warnings, but right now there are so many that we tend to
ignore them! One of the high runners is float equality warnings, another is
shadowed symbol warnings. My only objective here is a clean make.

On Thu, Jul 3, 2014 at 2:15 PM, px4dev [email protected] wrote:

Jean,

When passed to a function, this is actually more expensive (move from
vector register to GP register, arithmetic) than just stacking an argument
to the function.

In general, in-band magic values are bad; we should not use them.

On Jul 3, 2014, at 11:13 AM, Jean Cyr [email protected] wrote:

I was only referring to the case where 0 is assigned to a float to
indicate an uninitialized state (often the case in PX4 code). As in:

float x = 0.0f;
...
if (x == 0.0f) {
x = sensorValue();
}

—
Reply to this email directly or view it on GitHub.

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

[DrTon]

Lorenz, 1e-7 for ALL is not good at all. Do you know apriori units and valid values range? In some calculations intermediate values ~1e-7 may be normal. Band checks should be performed only if band is known, e.g. when using floats in mavlink message as flags.

BTW, in normal code this must not happen at all, division by near zero value should be performed only after check (e.g. for derivative calculation we know minimal acceptable dt). So final value check for NAN should be enough.

[DonLakeFlyer]

Lets separate the discussion into two parts:

1. Removing compiler warnings
2. Correctness of the existing code which is doing various things with float

My proposal is to use the fp_helpers macros to solve #1 while not attempting to solve problem #2. In doing so I will not change the current semantics or float usage pattern in the existing code. The macros then provide the mechanism to easily find all questionable float usage, just like the -Wfloat-equal compiler warning is doing now. And then the discussion can continue separately in what to do about #2. This way we can at least move forward more quickly on the compiler warnings.

On Jul 3, 2014, at 11:33 AM, Anton Babushkin [email protected] wrote:

Lorenz, 1e-7 for ALL is not good at all. Do you know apriori units and valid values range? In some calculations intermediate values ~1e-7 may be normal. Band checks should be performed only if band is known, e.g. when using floats in mavlink message as flags.

BTW, in normal code this must not happen at all, division by near zero value should be performed only after check (e.g. for derivative calculation we know minimal acceptable dt). So final value check for NAN should be enough.

—
Reply to this email directly or view it on GitHub.

[jean-m-cyr]

In 1) you propose to add further bloat by replacing what should be a simple comparison with extra floating point operations, just to eliminate compiler warnings???

Instead of:

#define is_exactly_zero_float(f) (!isfinite(1.0f/(f)))
#define is_exactly_zero_double(d) (!isfinite(1.0/(d)))

why not:

#define is_exactly_zero_float(f) (!((f < 0.0f)||(f > 0.0f)))
#define is_exactly_zero_double(d) (!((f < 0.0)||(f > 0.0)))

The compiler's optimization stage is smart enough to recognize that these are just equal 0 comparisons and will generate the same instructions as thought you had coded and simple check for 0.0, but will not generate the float equality warning.

[jean-m-cyr]

Oops... should have been #define is_exactly_zero_double(d) (!((d < 0.0)||(d > 0.0)))

[px4dev]

At the very least, let's just have one macro that works for both float and double arguments.

For the current platform, float and double are the same type. IFF you care about portability, consider using a combination of typeof() and decltype.

Please note that your test assumes that NAN and INF are zero.

[DonLakeFlyer]

decltype is C++ only. This needs to work in both C and C++ code

On Jul 4, 2014, at 10:20 AM, px4dev [email protected] wrote:

At the very least, let's just have one macro that works for both float and double arguments.

For the current platform, float and double are the same type. IFF you care about portability, consider using a combination of typeof() and decltype.

Please note that your test assumes that NAN and INF are zero.

—
Reply to this email directly or view it on GitHub.

[DonLakeFlyer]

All of this is to avoid using #pragma’s to turn off compiler warnings around a simple f == 0.0f comparison. Which to me seems like a waste of time. I’m going to go back to the pragma + inline method in fp_helpers. As I said I’m going to leave the code exactly as it is, just encapsulate it in inline helper functions to silence the warning. After that anyone else is welcome to change the code to their favorite floating point mechanism. Then I’ll move on to cleaning up the rest of the warnings.

On Jul 4, 2014, at 9:10 AM, Jean Cyr [email protected] wrote:

In 1) you propose to add further bloat by replacing what should be a simple comparison with extra floating point operations, just to eliminate compiler warnings???

Instead of:

#define is_exactly_zero_float(f) (!isfinite(1.0f/(f)))
#define is_exactly_zero_double(d) (!isfinite(1.0/(d)))

why not:

#define is_exactly_zero_float(f) (!((f < 0.0f)||(f > 0.0f)))
#define is_exactly_zero_double(d) (!((f < 0.0)||(f > 0.0)))

The compiler's optimization stage is smart enough to recognize that these are just equal 0 comparisons and will generate the same instructions as thought you had coded and simple check for 0.0, but will not generate the float equality warning.

—
Reply to this email directly or view it on GitHub.

[jean-m-cyr]

Hmm... I'm not sure it is legal to include a #pragma statement inside of a #define, or are you talking about using #pragma inside of an inline function? Don't forget that the inline specification is only a suggestion to the compiler which it may ignore. You might end up with a bunch of function calls to a bunch of duplicate instantiations of the is_exactly_zero function.

Seems to me that the #define mechanism would yield more predictable results.

[DonLakeFlyer]

It’s not. This will work in both C and C++ and end up being exactly what the code is now except for the warnings:

extern inline bool is_exactly_zero_float(float f);

extern inline bool is_exactly_zero_float(float f)
{
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wfloat-equal"
return f == 0.0f;
#pragma GCC diagnostic pop
}

extern inline bool is_exactly_zero_double(double d);

extern inline bool is_exactly_zero_double(double d)
{
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wfloat-equal"
return d == 0.0;
#pragma GCC diagnostic pop
}

On Jul 4, 2014, at 11:26 AM, Jean Cyr [email protected] wrote:

Hmm... I'm not sure it is legal to include a #pragma statement inside of a #define.

—
Reply to this email directly or view it on GitHub.

[px4dev]

(Don; hence recommending a mix of typeof() (C only) and decltype.)

Folks; we seem to be missing the point here. The reason these warnings are turned on is to help find and fix code that is doing bad things with floating point numbers.

Adding a mechanism for silencing the warnings is the wrong thing to do, because all you achieve is teaching people how to silence legitimate warnings when they come up. We disallow equality comparisons between floating point numbers because sufficiently often this is the wrong thing to do, and thus it should not be done simply as a matter of course.

The correct, and only, fix for these warnings is to amend the code to DTRT.

[DonLakeFlyer]

Ok. I’m going to move on from here. I’m going to clean up all the other compiler warnings and leave the float ones for someone else to deal with. I don’t think there is a consensus on what you say below as absolutely being true. Nor is there any consensus on how to correctly fix the code to legitimately avoid to warnings. Float comparison is obviously contentious. Which can be seen even in the compiler settings as well, with the fact the -Wfloat-equal is not included in the -Wall or -Wextra set of standard warnings. You have to explicitly turn it on. Someone who knows more about the codebase and what is actually intended is going to have to handle this.

On Jul 4, 2014, at 1:29 PM, px4dev [email protected] wrote:

(Don; hence recommending a mix of typeof() (C only) and decltype.)

Folks; we seem to be missing the point here. The reason these warnings are turned on is to help find and fix code that is doing bad things with floating point numbers.

Adding a mechanism for silencing the warnings is the wrong thing to do, because all you achieve is teaching people how to silence legitimate warnings when they come up. We disallow equality comparisons between floating point numbers because sufficiently often this is the wrong thing to do, and thus it should not be done simply as a matter of course.

The correct, and only, fix for these warnings is to amend the code to DTRT.

—
Reply to this email directly or view it on GitHub.

[LorenzMeier]

Don,

Your push is really appreciated. I think I can sit down tomorrow and eliminate a big share of them properly. Your suggested path makes perfect sense.