Skip to content
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

fix float64 precision division inaccuracy #150

Closed
wants to merge 1 commit into from

Conversation

Slach
Copy link

@Slach Slach commented Apr 25, 2017

see #149

@Slach Slach changed the title fix https://github.com/xeipuuv/gojsonschema/issues/149 fix float64 precision division inaccuracy May 3, 2017
@notjames notjames mentioned this pull request Sep 13, 2017
@ornotandrew
Copy link

@handrews Any comments on this? After reading #162 I'm not sure who is in charge, or if @xeipuuv is still around.

I'm having to pull from @Slach's branch to get this issue fixed under time pressure.

If you're okay with the approach of this commit, can we please merge it? Otherwise, it's probably a good idea to start a discussion about how this should be solved properly. A different approach could be to use something like big.Float everywhere that float64 is currently used, but that seems like a pretty hairy refactor.

@handrews
Copy link

@wraithy I'm really just a JSON Schema consultant, I've never written a line of Go, so I'm not sure why I have merge permissions. Might be an accident. I'm really not comfortable managing this project, I only dropped in to answer schema questions and advocate for implementing newer JSON Schema drafts.

@ashb
Copy link
Contributor

ashb commented Nov 20, 2017

@wraithy Do you know how other JSONschema libs deal with this?

This sort of problem is to do with how JSON stores numbers -- it only has floats, and at some point your (money?) column is going to lose accuracy when you least expect it.

The change you've got to fix it is... not great because of rounding issues it could cause elsehwere. multipleOf possible doesn't make sense in terms of non-integer numbers because although you've written 19.99 once it is parsed to a floating point number the actual value is 19.98999999999999843681 (`fmt.Printf("%.20f\n", 19.99).

In your specific example since your fields are dealing with currency you should used fix point numbers (i.e store the numbers as cents, not dollars) otherwise you are going to have rounding issues somewhere in your code and be out by a cent or two. See for more info https://stackoverflow.com/questions/3730019/why-not-use-double-or-float-to-represent-currency/3730040#3730040

@handrews, thats a spec quiection for you to chime in on: perhaps it is worth adding a note about multipleOf on floating point numbers will be in-exact and should be avoided, or that implementers should use an epsilon test rather than exact when dealing with non-integers.

@ornotandrew
Copy link

@ashb As you pointed out, the trick is to not parse your JSON numbers into a float, ever. I've only worked with one other JSONschema library (in Python), and it dealt with this issue by allowing you to supply a custom object that numbers should be parsed into. In Python, this is typically decimal.Decimal, and in golang it's math/big.Float.

These objects avoid the problem with floating point representations by keeping the individual components of the number (mantissa, exponent) in separate bits of memory so that they can scale as needed.

Storing monetary values as cents is one solution, but storing whole values is perfectly fine as long as you use the correct (i.e. not float) storage container at each stage in the process.

The golang program I'm working on mostly deals with money, and floats are not used anywhere, with one exception in this library, on this line: https://github.com/xeipuuv/gojsonschema/blob/master/validation.go#L762

(Which by the way, is interesting, since json.Number is correctly used everywhere else)

I accepted the current solution since it provides enough accuracy to deal with most currencies, and didn't have time to do it differently. I might have time to do it now, but frankly my issue is that @xeipuuv will likely not merge it.

@handrews
Copy link

@ashb for multipleOf floating point issues, see json-schema-org/json-schema-spec#312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants