Floating point equality - revisited

[cburlacu]

This is related to #185. I know that comparing floats implementation is a delicate subject, but the current behavior can be improved.

Suppose this code (suppose no errors occurs):

    using json = nlohmann::json;
    json j1, j2;
    j1["test_double"] = 3.12345;
    j1["test_string"] = "test string";
    j1["test_array"] = (std::vector<int>){12345, 12346, 2345, 23456, 3456, 4567, 5678, 6789};
    j1["test_float"] = 3.123f;

    j2 = json::parse(j1.dump());

    json::iterator it = j1.begin();
    bool areEqual = true;

    for( ; it != j1.end(); ++it) {
        auto otherVal = j2[it.key()];
        auto thisVal = it.value();
        if(otherVal != thisVal) {
            areEqual = false;
            break;
        }
    }

When the float properties are compared, the result is false which is not what I was expecting.

                case value_t::number_float:
                    return (lhs.m_value.number_float == rhs.m_value.number_float);

this comparison translates to 3.123f as:
lhs = 3.1229999065399201
rhs = 3.122999906539917

Tested both on Ubuntu 17.04 & macOS Sierra with same results.

[nlohmann]

You store the float number 3.123f in the JSON value. Internally, floating-point numbers are stored as json::number_float_t which defaults to double. This number is serialized to 3.12299990653992 and then parsed to the double value 3.1229999065399201.

If you start with a double instead of a float, the round-tripping works.

[cburlacu]

Double is not working either:
j1["small_double"] = DBL_EPSILON; // or DBL_MIN, or even M_PI

You cannot assume that you'll have nice values. I'm not complaining about precision/accuracy/etc, but to compare them similar to this:
(fabs(left - right) < epsilon);

[nlohmann]

The problem is that such a comparison under the hood of the operator== may be really surprising.

[mariokonrad]

Sorry to chime in. In my library marnav, I do also the abs(a-b)<=epsilon test for this, but not as an overload, but in a separate function template. See here.

To me, it seems a common pattern. The only problem I see from the top of my head is when operands are denormalized, inf or NAN.

The question I my opinion is, should basic_json provide such an operator, or should it return the floating point value as is and let the user deal with it, knowing that floating point comparisons come with pitfalls. Personally, I prefer the latter, hence the free function template.

[cburlacu]

It seems that the issue is related to serializing process.

    j2 = json::parse(j1.dump());
    j3 = json::parse(j1.dump());

j2 is equal with j3, but not with j1.

[nlohmann]

@mariokonrad Thanks for the hint. I'm not sure whether it would make sense to offer such a function in the library, but rather add it to the documentation of operator==.

@cburlacu Serialization is another aspect. We limit the number of digits we serialize. But even then, the conversion from float to double may be problematic.

[mariokonrad]

The most important thing IMHO is that it is documented properly, i.e. the user should not need to guess.

For serialization, it might be possible to use std::numeric_limits::digits10 to represent floating point numbers. The example shown above:

this comparison translates to 3.123f as:
lhs = 3.1229999065399201
rhs = 3.122999906539917

In this case of float, numbers of digits are way above what float can represent without any loss due to rounding errors, which is 6, double is 15, long double is 18. If the numbers above are restricted to 6 digits (in case for float), they are perfectly fine.

[nlohmann]

We actually use std::numeric_limits<number_float_t>::digits10, see https://github.com/nlohmann/json/blob/develop/src/json.hpp#L6662.

[mariokonrad]

Oh, sorry, missed it.

And you are completely right, conversion from float to double may introduce nasty stuff.

Though, still not sure if nlohmann/json should handle problems that come with float/double. Is it really its job? Convenient maybe...

[nlohmann]

I agree that we should not handle these issue. And I would not like the following code:

// assume some values for d1 and d2
double d1 = ...;
double d2 = ...;

// assume d1 and d2 are not equal
bool b1 = d1 == d2;

json j1 = d1;
json j2 = d2;

// assume j1 and j2 are now equal, because we only compare wrt. an epsilon
bool b2 = j1 == j2;

I think b1 and b2 should have the same value, and thus basic_json::operator== should be implemented with a "vanilla" version of operator==(double, double).

[mariokonrad]

I agree.

[jaredgrubb]

Agree. I would expect that testing '==' is true <=> a manually-run recursive test of '==' is true. Anything else would be surprising to a user.

I do think there's utilitiy in having an "equivalence" test that is looser than strict equality, though.

[nlohmann]

In any case, I shall add a note to the documentation of operator==. I am not sure whether we need a special "equivalence" function for floating-point numbers in the library.