Division Loss of Precision

[matthewoestreich]

I mentioned this in #30 but figured opening an issue was more appropriate.

Implementation of DivAssign causes issues with trunc_div. I could be wrong, but I believe this is because the DivAssign implementation promotes to float prior to dividing, which causes the i128::MAX to lose some precision.

For example, notice the last digit (3 vs 4):

println!("{}", i128::MAX / 2); 
//-> 85070591730234615865843651857942052863
println!("{:?}", Value::SignedBigInt(i128::MAX).trunc_div(2)); 
//-> UnsignedBigInt(85070591730234615865843651857942052864)

Demo outlining root issue

Click to view demo

println!("\t=============== RAW VALUES ==============\n");
println!("i128::MAX = {}", i128::MAX);
let a = i128::MAX as f64;
println!("i128::MAX as f64 = {a:e} [no fmt] = {a}");
println!("\n\t============== CONVERSION TO f64 PRIOR TO DIVISION ==============\n");
println!("i128::MAX / 2 = {}", i128::MAX / 2);
let b = (i128::MAX as f64) / 2.0;
println!("(i128::MAX as f64) / 2.0 = {b:e} [no fmt] = {b} <-- precision is lost!");
println!("\n\t============== CONVERSION FROM f64 BACK INTO i128 ==============\n");
let c = b as i128;
println!("((i128::MAX as f64) / 2.0) as i128 = {c:e} [no fmt] = {c} <-- loss of precision reflected");

//        =============== RAW VALUES ==============
//
// i128::MAX = 170141183460469231731687303715884105727
// i128::MAX as f64 = 1.7014118346046923e38 [no fmt] = 170141183460469230000000000000000000000
//
//        ============== CONVERSION TO f64 PRIOR TO DIVISION ==============
//
// i128::MAX / 2 = 85070591730234615865843651857942052863
// (i128::MAX as f64) / 2.0 = 8.507059173023462e37 [no fmt] = 85070591730234620000000000000000000000 <-- precision is lost!
//
//        ============== CONVERSION FROM f64 BACK INTO i128 ==============
//
// ((i128::MAX as f64) / 2.0) as i128 = 8.5070591730234615865843651857942052864e37 [no fmt] = 85070591730234615865843651857942052864 <-- loss of precision reflected

Possible fix

Implementation of DivAssign for reference:

Click to view existing DivAssign

impl<Rhs> ops::DivAssign<Rhs> for Value
where
    Rhs: Into<Value>,
{
    fn div_assign(&mut self, rhs: Rhs) {
        self.promote_to_float();
        let mut rhs = rhs.into();
        rhs.promote_to_float();
        *self = dispatch_operation!(self, rhs, n, |rhs| (*n).checked_div(rhs).map(Value::from))
            .unwrap_or_else(|| {
                self.promote();
                dispatch_operation!(self, rhs, n, |rhs| Value::from(*n / rhs))
            });
    }
}

Changing the implementation to the code below solves the issue described above.

• I didn't want to make this change because I saw comments mentioning that divison always produces a float
• This change would mean all division would follow the same promotion rules as the AddAssign/SubAssign/MulAssign/RemAssign implementations
• This change also causes a bunch of tests to fail for the same promotional reason (not all divison would produce a float)

Click to view DivAssign fix

impl<Rhs> ops::DivAssign<Rhs> for Value
where
    Rhs: Into<Value>,
{
    fn div_assign(&mut self, rhs: Rhs) {
        let mut rhs = rhs.into();
        *self = dispatch_operation!(self, rhs, n, |rhs| (*n).checked_div(rhs).map(Value::from))
            .unwrap_or_else(|| {
                self.promote();
                dispatch_operation!(self, rhs, n, |rhs| Value::from(*n / rhs))
            });
    }
}

Thoughts?

[coriolinus]

I have seen this issue but not yet had time to look into it thoroughly. Please be patient.

[matthewoestreich]

@coriolinus no worries - after having some time to come back to this, I think I see why you always produce a float when dividing.. should have been obvious but I didn't see it before (embarrassing!).

Therefore, my proposed solution doesn't actually resolve the issue as a whole, it actually creates a new one.

Idk why, it just bothers me that if the result is an integer it still produces a float when it doesn't need to be a float. That said, I understand why it does now...

// It kills me that this doesn't produce `Value::UnsignedInt(5)`
Value::UnsignedInt(10) / Value::UnsignedInt(2);
// I now see why you always produce a float for div
Value::UnsignedInt(1) / Value::UnsignedInt(2); // Value::Float(0.5)

We should still be able to fix the precision loss issue though, no? Thoughts?