Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Loss of precision with the f64 conversion #267

Closed
pimeys opened this issue Sep 10, 2020 · 3 comments · Fixed by #269
Closed

Loss of precision with the f64 conversion #267

pimeys opened this issue Sep 10, 2020 · 3 comments · Fixed by #269

Comments

@pimeys
Copy link
Contributor

pimeys commented Sep 10, 2020

Hi. I've been investigating a bug today about a loss of precision, and I pointed it now to the mantissa algorithm in Decimal's to_f64. This is the code that fails in our system, and I'd like to know is the loss of precision expected or can it be a bug?

use num::ToPrimitive;
use rust_decimal::prelude::*;

fn main() -> anyhow::Result<()> {
    let s = "1.59283191";
    let d = Decimal::from_str(s)?;
    assert_eq!(s.parse::<f64>()?, d.to_f64().unwrap());

    Ok(())
}

What I can do in our case is to just go from string to float, keeping the precision and our users happy. This will not be as performant though, so I'm interested is the mantissa algorithm supposed to lose some precision, or could we maybe fix it?

> uname -a
Linux lyndon 5.8.7-arch1-1 #1 SMP PREEMPT Sat, 05 Sep 2020 12:31:32 +0000 x86_64 GNU/Linux
> rustc --version
rustc 1.46.0 (04488afe3 2020-08-24)

rust_decimal version 1.7.0.

The PR: #235

Ping @paupino @hengchu

@hengchu
Copy link
Contributor

hengchu commented Sep 10, 2020

Hmm I think the loss of precision happens at this step:

rust-decimal/src/decimal.rs

Lines 2663 to 2664 in 916bf3d

let frac_part = mantissa % precision;
let frac_f64 = (frac_part as f64) / (precision as f64);

This division converts to two u128 into f64 first, and then performs the division. There may be a better way to compute the fraction part, but it will probably involve some bit-level mangling of the actual f64 representation.

Do we know what algorithm rust's stdlib uses for parsing String into f64? I can try to take a look at that and see if it's possible to guarantee identical outputs between the two routes: Decimal -> String -> f64 and Decimal -> f64.

@paupino paupino self-assigned this Sep 11, 2020
@paupino
Copy link
Owner

paupino commented Sep 11, 2020

That particular calculation actually looks like it's correct in my models here. The calculation causing the issue is this:

(integral_part as f64) + frac_f64

In fact, the result can be replicated by:

    let i = 1_u128;
    let j = 0.59283191_f64;
    println!("{}", (i as f64) + j);

That being said, as you mention - we could potentially leave the conversion to f64 till after ALL calculations are done? I'll play with a couple of options and see if anything sticks.

@paupino paupino removed their assignment Sep 11, 2020
@paupino
Copy link
Owner

paupino commented Sep 11, 2020

In fact, this is not even a result of the f64 conversion. The following will also give the same result:

    let i = 1_f64;
    let j = 0.59283191_f64;
    println!("{}", i + j);

outputs 1.5928319100000001

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

Successfully merging a pull request may close this issue.

3 participants