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

Small error when parsing long float value #83

Open
Mark-Simulacrum opened this issue Aug 1, 2016 · 12 comments
Open

Small error when parsing long float value #83

Mark-Simulacrum opened this issue Aug 1, 2016 · 12 comments
Labels

Comments

@Mark-Simulacrum
Copy link

JavaScript's JSON.parse doesn't lose precision from my tests.

Test case below:

#[test]
fn large_precision() {
    assert_eq!(parse("0.004750000000000001").unwrap().as_f64().unwrap(), 0.004750000000000001);
}
@Mark-Simulacrum
Copy link
Author

If it helps, I have quite a few cases similar to this one. Let me know if they'd be helpful.

@dtolnay
Copy link

dtolnay commented Aug 1, 2016

This error comes from the fact that in floating point arithmetic, 4750000000000001f64 * 10e-19f64 is 0.004750000000000002. @maciejhirsz Serde uses 4750000000000001f64 / 10e19f64 because it is more accurate.

@maciejhirsz maciejhirsz added the bug label Aug 1, 2016
@maciejhirsz maciejhirsz added this to the 0.10.1 milestone Aug 1, 2016
@maciejhirsz
Copy link
Owner

Thanks for the tip @dtolnay.

@Mark-Simulacrum sure, I'd gladly include any edge cases in my tests.

@Mark-Simulacrum
Copy link
Author

Mark-Simulacrum commented Aug 1, 2016

Warning: the files are large; so wget vs opening in browser is a good idea (Chrome, at least, was unable to do anything beneficial with the file: no display and lots of CPU/memory usage).

Some of these may be false (actually correct serialization), but this is a sort | uniq | diff -u of the output of the program that "found" this error: https://gist.githubusercontent.com/Mark-Simulacrum/280d5b4530ae68f958b3b3542bb613e1/raw/120c3ba89daa9a526225496673e5e19f396f0de3/rust-json.diff. I'm not sure how helpful this diff is, but it's a start.

Also, I've found that I'm not 100% sure that Serde's implementation is equivalent to the JS version either; here's the same type of file for the Serde impl: https://gist.githubusercontent.com/Mark-Simulacrum/05431fd1e1048ee1924879cdde2e0fb9/raw/462391ec7e17acb1ae4a32ae852bd45bbda4d2ab/serde.diff. Serde seems to have less errors, but there do appear to still be errors.

@maciejhirsz
Copy link
Owner

Thanks for that, very helpful!

@Mark-Simulacrum
Copy link
Author

Mark-Simulacrum commented Aug 1, 2016

@maciejhirsz I'm thinking we should either reopen this or I can move this into #85; still seeing errors with 0.10.1. Diff included (12M) this time I'm nearly 97% certain on the errors since I included more information in the output: https://gist.githubusercontent.com/Mark-Simulacrum/0a26fadc129dc99c441a3df0b42ae9d7/raw/18c0e5bd85939841446706614173cec6c2be51e5/erroring.diff

@Mark-Simulacrum
Copy link
Author

I can also provide the raw JSON files (they're actually already on Github (see rust-lang-nursery/rustc-perf and the instructions there as to how to get the "data" directory) if that helps, although from the 1-2 values I've checked everything matches (JS processed the number correctly).

@Mark-Simulacrum
Copy link
Author

@dtolnay I reran my serde_json version and diffed that with the latest diff link, and Serde now has exactly the same problems as json-rust. If you'd like I can file an issue with the diff/errors on serde_json's repo.

@maciejhirsz maciejhirsz reopened this Aug 1, 2016
@maciejhirsz
Copy link
Owner

@Mark-Simulacrum not sure if I'm reading this right, did the error rate went up with 0.10.1? The first diff is a fraction of this one.

@maciejhirsz maciejhirsz removed this from the 0.10.1 milestone Aug 1, 2016
@Mark-Simulacrum
Copy link
Author

Well, the diff will be longer since this time I'm not feeding the results through uniq; that way I can be nearly 100% certain that the results aren't false positives. I've recomputed the results just in case: this is the non-unique version and this is the unique version.

Note that the unique version is much smaller than before (and what I gave you before was a unique version). However, the unique version shows that in most cases in my data set, when a precision error is encountered, it seems to be encountered with a few close-lying floats; so the diff is a little harder to interpret.

@Mark-Simulacrum
Copy link
Author

I've come up with the below as a sample of what may be a good alternative to manual testing (at least for simple cases to generate). This quickly fails the floats test but passes both signed and unsigned integers (which makes sense).

extern crate json;

#[macro_use]
extern crate quickcheck;

use json::JsonValue;

quickcheck! {
    fn unsigned_integers(n: u64) -> bool {
        JsonValue::from(n).as_u64().unwrap() == n
    }

    fn signed_integers(n: i64) -> bool {
        JsonValue::from(n).as_i64().unwrap() == n
    }

    fn floats(n: f64) -> bool {
        JsonValue::from(n).as_f64().unwrap() == n
    }
}

@dtolnay
Copy link

dtolnay commented Aug 2, 2016

Thanks @Mark-Simulacrum, I filed serde-rs/json#128 on our end.

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

No branches or pull requests

3 participants