pboettch / json-schema-validator

JSON schema validator for JSON for Modern C++
Other
463 stars 133 forks source link

`multipleOf` fails on double-value #293

Closed dhmemi closed 5 months ago

dhmemi commented 7 months ago
void test () {
  nlohmann::json schema = {{"type", "number"}, {"multipleOf", 0.001}};
  nlohmann::json data = 0.3 - 0.2; // 

  const nlohmann::json_schema::json_validator validator(schema);
  validator.validate(data);
} 

This simple test case will throw an error due to floating point computational error:

At  of 0.09999999999999998 - instance is not a multiple of 0.001000

Although, strictly speaking, this is not a bug. But it makes the multipleOf validation for floating-point numbers completely unusable in real-world usage scenarios.

Is there any suggestions on how to solve this problem?

pboettch commented 7 months ago

Sadly this is nothing related to this library but rather a C/C++ datatype issue.

We are already using

    bool violates_multiple_of(T x) const
    {
        double res = std::remainder(x, multipleOf_.second);
        double eps = std::nextafter(x, 0) - static_cast<double>(x);
        return std::fabs(res) > std::fabs(eps);
    }

to minimize floating point derivation. Seems not to be enough in your case.

Actually it is strange. Is this maybe a platform-bug? Are you x86?

dhmemi commented 7 months ago

This is not a platform-specific bug; it is caused by the binary encoding and representation mechanism of floating-point numbers. This bug exists on most platforms, including x86/64/ARM.

The following calculation method can resolve the problem caused by floating-point calculation errors:

    bool violates_multiple_of(T x) const
    {
        double multiple = x / multipleOf_.second;
        double error = std::abs( (multiple - std::round(multiple)) * multipleOf_.second );
    return error <= std::numeric_limits<T>::epsilon;
    }

Is this acceptable for this repository ?

pboettch commented 7 months ago

If you could create a PR with this change and adding your case (and other) with a test, I think this can be integrated here.

pboettch commented 7 months ago

maybe limiting it to T = double.