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.

[nlohmann]

Added documentation, see 91e0032#diff-d525c458d12b63b74e3cf18acbbe25bbR12353.

[danlipsa]

I apologize for asking a question on an old issue. I am trying to use the function suggested in the documentation: is_same. It's not clear to me where to I pass this function.
The idea is clear: I have to replace the default double operator== but I have troubles making it work. I tried:

• including the is_same function in my code as written.
• renaming it to operator==
• Writing directly an bool operator==(json::number_float_t& lhs, json::number_float_t& rhs)
  The first two don't get called, the third gives an error saying you cannot override operator== for double. Any suggestions? Thanks!

[nlohmann]

I don't think you can replace the existing behavior for the == comparison of two json values. Instead, you need to define your own comparison function like bool my_compare(const json& lhs, const json& rhs) and call this instead of operator==.

[danlipsa]

Thanks for the suggestion. Yes, this worked nicely.
https://gitlab.kitware.com/danlipsa/vtk/-/blob/cesium3dtiles/IO/Cesium3DTiles/Testing/Cxx/TestCesium3DTilesWriter.cxx#L283

I would update the documentation
https://json.nlohmann.me/features/types/number_handling/#number-comparison
as the suggestion given there is slightly different. It suggests using is_same OR
bool my_equal(const_reference lhs, const_reference rhs). This led me to believe I could use is_same by itself. Also the parameters for my_equal are different than what you suggested here and what I ended up using.

As a whish list, it would be nice to be able to replace the equality operator for double. It would eliminate the need to write an additional jsonEqual that is probably very similar with the original except for the double equality.
Thanks again!