-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Float precision errors with multipleOf #187
Comments
From the multipleOf JSON schema specification:
Therefore, if any change should be made, I would suggest that the change would be to throw an error or provide a warning if the parameter is a number less than 0, as that is an invalid schema. Is there any case where floating point breaks down with numbers greater than 0? |
+1 I am running into this issue myself attempting to validate randomly generated numbers (floats) within a schema that are a multiple of 0.015. I would suggest scaling the floating values to their integer equivalents when performing the check for "multipleOf/DivisibleBy" like so: 92.415 / 0.015 = 6161.000000000001 //JavaScript floating point precision quirk
//vs
92415 / 15 = 6161 or an example if instance value is at less precision than multipleOf: 99.78 / 0.015 = 6652 //instance value is 2 precision while the multipleOf is 3
99780 / 15 = 6652 //Must add zero's to instance value to equal precisions on both sides
|
Hmm, understood, but the standard also says: http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.1 So how should we go about fixing this, given the IEEE 754 quirk (it's not just JS)? |
Correct. Both examples I gave, if calculated correctly by JavaScript, would result in integer values with no remainders if the floating point quirks weren't there. Hence, that is why I made the suggestion above. Integer operations do not have those quirks. So if you can find a solid implementation of converting the floats into their integer equivalents, that should alleviate having to deal with floats at all for evaluation purposes. However, finding a solid implementation for converting the floats is tricky as I look into it... Converting the precision of the instance value is pretty straight forward, if needed, by using the Object.prototype.toFixed() method. Determining the precision of the multipleOf/DivisibleOf values is more complicated, especially when dealing with scientific notation. From the small amount of searching I've done so far, there are a lot of different methods proposed, but none of the them seem to be full proof. My thoughts are that it would look something along the lines of: const stepPrecision = determineStepPrecision(schema.multipleOf); //How to reliably do this????
const instanceAsInt = Number(instance.toFixed(stepPrecision).replace('.', ''));
const stepAsInt = Number(schema.multipleOf.toFixed(stepPrecision).replace('.', ''));
if (instanceAsInt / stepAsInt % 1) {
//INVALID
} Let me research a little more and see if I can come up with anything... |
So this is what I have come up with after researching last night. It appears that using string parsing is the most full proof way of determining the decimal precision of a number. I tried to find a mathematical way to achieve it but all of those solutions were not 100% accurate (all subject to the rounding errors). The following function is what I have derived to find the number of decimal places in a provided number: const getDecimalPlaces = function(number) {
let decimalPlaces = 0;
if (isNaN(number)) return decimalPlaces;
const valueString = Number(number).toString();
const decimalPortion = valueString.split('.')[1];
//No need to check if there is no decimal point
if (decimalPortion) {
//Check for scientific notation.
const decimalParts = decimalPortion.split('e');
if (decimalParts.length === 2) {
//If the notation is on the positive side, no need to check
//further as it's not a fractional number. Otherwise, store
//the number of base decimal places indicated in the notation
if (decimalParts[1][0] !== '-') {
return decimalPlaces;
} else {
decimalPlaces = Number(decimalParts[1].slice(1));
}
}
//Add the decimal places right of the decimal point; this includes the
//remainder of decimal places in the scientific notation
decimalPlaces += decimalParts[0].length;
}
return decimalPlaces;
} As mentioned in my previous post, I was going to use the "toFixed" method to convert the floats to integers. However, the "toFixed" method only works out to 20 decimal places. So, rather then mess with that, the most recommended way to convert the floats to integer is just multiply them by 10 to the power of the number of decimal places they have. With that in mind, our multipleOf/divisibleBy check becomes: const instanceDecimals = getDecimalPlaces(instance);
const multipleOfDecimals = getDecimalPlaces(schema.multipleOf);
const maxDecimals = Math.max(instanceDecimals , multipleOfDecimals );
const multiplier = Math.pow(10, maxDecimals);
if (((instance* multiplier) % (schema.multipleOf * multiplier))) !== 0 {
//INVALID
} I have done a fair amount of testing with this and so far it hasn't missed a beat. I will do some more when I get the chance just to make sure and test it out completely. Update: Made small edits to source code to put it in the context of JSONSchema. |
Ok...after more testing, I did find some issues with the original code. However, they were easily sorted and I believe the final solution comes down to this: //helpers.js ???
exports.getDecimalPlaces = function getDecimalPlaces(number) {
let decimalPlaces = 0;
if (isNaN(number)) return decimalPlaces;
if (typeof number !== 'number') {
number = Number(number);
}
const parts = number.toString().split('e');
if (parts.length === 2) {
if (parts[1][0] !== '-') {
return decimalPlaces;
} else {
decimalPlaces = Number(parts[1].slice(1));
}
}
const decimalParts = parts[0].split('.');
if (decimalParts.length === 2) {
decimalPlaces += decimalParts[1].length;
}
return decimalPlaces;
}
//attribute.js
validators.multipleOf = function validateMultipleOf (instance, schema, options, ctx) {
if (typeof instance !== 'number') {
return null;
}
if (schema.multipleOf == 0) {
throw new SchemaError("multipleOf cannot be zero");
}
var result = new ValidatorResult(instance, schema, options, ctx);
const instanceDecimals = helpers.getDecimalPlaces(instance);
const multipleOfDecimals = helpers.getDecimalPlaces(schema.multipleOf);
const maxDecimals = Math.max(instanceDecimals , multipleOfDecimals );
const multiplier = Math.pow(10, maxDecimals);
if (Math.round(instance * multiplier) % Math.round(schema.multipleOf * multiplier) !== 0) {
result.addError({
name: 'multipleOf',
argument: schema.multipleOf,
message: "is not a multiple of (divisible by) " + JSON.stringify(schema.multipleOf),
});
}
return result;
}; The above would need to be implemented in the 'divisibleBy' method as well. |
@manahga I'm also hoping for a fix to this. Your approach looks good to me. Are you planning on making a PR out of it soon? Or if you don't have time, do you mind if I submit a PR based on your code? |
Yes please! I'm all for this, but really pushed for time. If you create a pull request and include some tests showing the code works as intended, I will happily merge this in. |
I also wrote a PR to cover this case in the JSON-Schema-Test-Suite that you use, but I think it'll be rejected as the owner argued that the floating point problems with multipleOf are the correct behaviour, as JSON specs say that JSON can be deserialised to floats ¯_(ツ)_/¯ |
@tdegrunt no pressure or anything 😄 but do you have any idea if and when this PR might get merged, and when the next npm release might be made? I'm hoping to get this change included in a PR for react-jsonschema-form, as it's causing an issue in the product we're writing. but if not going to happen soon, I can make do with a workaround. |
Like I said, busy :) Thank you both for your efforts! |
wonderful, thank you! hope you have a less busy weekend 😄 |
- modulo doesn't work with floating point values in many cases, e.g. `0.3 % 0.1 == 0.09999999999999998` - port implementation from: tdegrunt/jsonschema#187 (comment) - add tests for int/float multiple_of values - update history with pr/author
One cannot use the
multipleOf
constraint with floats as it will fail in many cases due to classic floating point errors:Because:
jsonschema version: 1.1.0
The text was updated successfully, but these errors were encountered: