rust-num / num-rational

Generic Rational numbers for Rust
Apache License 2.0
144 stars 51 forks source link

format_as_decimal implementation #39

Closed dnsl48 closed 5 years ago

dnsl48 commented 6 years ago

Fixes #10

Hazardius commented 6 years ago

For now - just took a quick look on it and it looks really nice. I just have one thing that I can bring from PR #37 . As @cuviper pointed out in it - if (as in your tests) we gave precision 64 - maybe we should have the following 0's in the String representation. Eg. 1/1 should return 1.000...(64 zeroes) since we asked for 64 digits precision. Same for 1/2, 1/4, 1/5 and others. Of course precision could be interpreted either as an exact precision or maximum allowed precision. In the end, out of those two I'd prefer to get exact precision by default and that's why I changed my implementation to return String with those zeroes.

I'll try to read the whole PR with more focus this week, but the next two nights I'm working overtime so I'm not sure if I can to do it before Wednesday evening.

Nemo157 commented 6 years ago

For my use cases I would much prefer having the precision be a maximum, and not have trailing zeroes. I would love to also have a function like fn try_as_decimal_string(&self, precision: usize) -> (String, bool) that returns the value out to the max precision + whether the value fit in the precision specified; so that imprecise values can be marked in some way when displayed (probably would be better to use some enum rather than a bool, but I can't think of a good formulation for it right now).

dnsl48 commented 6 years ago

Since fmt::Result is a unit we can return bool instead. I've made the patch so now fn format_as_decimal returns completeness of the value. Regarding the trailing zeroes (as well as leading zeroes and other formatting stuff), I think we can state that this is a representational detail and should be implemented in the Display trait so that whoever needs it can do the formatting accordingly. I don't want it to be a part of this particular PR, because it is a bit different issue.

Nemo157 commented 6 years ago

Thanks a lot, this is working great in my trial 😄

dnsl48 commented 6 years ago

Well, after another thought it feels to be more flexible if we return a remainder of the division, not an indicator whether it's complete or not. And then, outside we can check with .is_zero if necessary.

dnsl48 commented 6 years ago

Whoever may need this functionality while the PR is making it, can use the fraction crate, which is the origin of the code in this PR. Particularly you may look at fraction::division::divide_to_string or the other functions in the division module. Enjoy.

cuviper commented 6 years ago

On trailing zeros, my expectation is that we'd behave just as f64 precision does for the same value (apart from mantissa rounding errors). For example:

fn main() {
    println!("{:.6}", 1.0/5.0);
    println!("{:.6}", 1.0/6.0);
    println!("{:.6}", 1.0/7.0);
}
0.200000
0.166667
0.142857
dnsl48 commented 6 years ago

Yes, that sounds reasonable. However, that implies we can achieve a result without trailing zeroes with not specifying a particular precision, which is not the case in this particular PR. To do so we'll have to have a hardcoded default precision which we use when a user doesn't pass it through the format parameter.

On the other hand, I believe that leading and trailing zeroes are a presentational detail and when necessary could be achieved through using width and align formatting parameters (e.g. {:0<8.6}). In that case we can also rely on the precision defined as the switch between outputting the decimal or rational number.

That said, if we make the output without trailing zeroes possible for only undefined precision, we're limiting the possible range of values to the default precision value, which is less flexible.

I cannot see the obvious winner in those 2 approaches. We either choose to be aligned with the f64 type behaviour, but become less flexible, or we leave the leading and trailing zeroes to be a presentational detail, which lets us be more flexible, but then we have some discrepancies with the stdlib formatting implementation for floating types.

My personal preference was to be more flexible, so that I didn't want to bloat this PR with the presentation details implementation and postponed it to a separate PR. However, I will completely understand if we choose to be aligned with the stdlib, sacrificing the flexibility.

dnsl48 commented 5 years ago

On trailing zeroes... Unfortunately, digging deeper in the Display and Formatter functionality I found out it's not really that flexible. I've added an explicit flag to choose whether we need trailing zeroes, or not.

dnsl48 commented 5 years ago

Sorry, I've lost my interest. All this stuff and even more is implemented in the fraction crate. It's open source and anyone who wants can use it, so I don't see much value in pushing it here anymore.

Otherwise, whoever wants can take this PR and patch it with any formatting they desire. Here's the algorithm extracted into a single reusable function:

    /// Calculate decimal whole and fractional parts
    /// and pass them to the consumer
    ///
    /// Consumer first invocation will always have the whole part of the resulting number.
    /// All the following calls pass the fractional part digit by digit.
    /// Consumer should return false to stop calculation before the fractional part
    /// is complete.
    /// Returns false if the consumer stopped calculation,
    /// otherwise returns true if the fractional part has been calculated completely
    fn calculate_decimal<C, E>(&self, mut consumer: C) -> Result<bool, E>
    where
        T: CheckedMul,
        C: FnMut(T) -> Result<bool, E>
    {
        let (integer, mut remainder): (T, T) = self.numer().div_rem(self.denom());

        if !consumer(integer)? {
            return Ok(false);
        }

        if !remainder.is_zero() {
            let divisor = self.denom();

            // Build the number 10 from scratch so we can handle both signed and unsigned types (T of i8 or u8)
            let mut _10 = T::one() + T::one() + T::one(); // 3
            _10 = _10.clone() * _10 + T::one(); // 10

            loop {
                if remainder.is_zero() {
                    return Ok(true);
                }

                let (rem, digit) = remainder.checked_mul(&_10).map_or_else(
                    || {
                        // Capacity of T is not enough to multiply the remainder by 10
                        let (reduced_divisor, reduced_divisor_rem) = divisor.div_rem(&_10);

                        let mut digit = remainder.div_floor(&reduced_divisor);
                        let mut remainder = remainder.clone();

                        remainder = remainder - digit.clone() * reduced_divisor.clone();

                        let mut red_div_rem_diff =
                        (reduced_divisor_rem.clone() * digit.clone()).div_rem(&_10);

                        loop {
                            if red_div_rem_diff.0 > remainder {
                                digit = digit - T::one();
                                remainder = remainder + reduced_divisor.clone();
                                red_div_rem_diff =
                                (reduced_divisor_rem.clone() * digit.clone()).div_rem(&_10);
                            } else {
                                break;
                            }
                        }

                        remainder = remainder - red_div_rem_diff.0;
                        remainder = remainder * _10.clone();

                        if red_div_rem_diff.1 > remainder {
                            digit = digit - T::one();
                            remainder = remainder + divisor.clone();
                        }

                        remainder = remainder - red_div_rem_diff.1;

                        (remainder, digit)
                    },
                    |mut remainder| {
                        let digit = remainder.div_floor(divisor);
                        remainder = remainder - digit.clone() * divisor.clone();

                        (remainder, digit)
                    },
                );

                remainder = rem;

                if !consumer(digit)? {
                    return Ok(false);
               }
            }
        }

        Ok(true)
    }