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

format_as_decimal implementation #39

Closed
wants to merge 4 commits into from

Conversation

dnsl48
Copy link
Contributor

@dnsl48 dnsl48 commented Oct 7, 2018

Fixes #10

  • Adds pub fn format_as_decimal which prints into any fmt::Write
  • Adds pub fn as_decimal_string -> String, which calculates the string length, allocates it and builds the number
  • No overflows, infinite lossless precision
  • Algorithm correctness has been tested with over 65k numbers (all combinations of u8 from 1 and up to 255 for both numerator and denominator)

@dnsl48 dnsl48 changed the title format_as_decimal initial implementation format_as_decimal implementation Oct 7, 2018
@Hazardius
Copy link

Hazardius commented Oct 8, 2018

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.

src/lib.rs Outdated Show resolved Hide resolved
@Nemo157
Copy link
Contributor

Nemo157 commented Oct 23, 2018

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
Copy link
Contributor Author

dnsl48 commented Oct 23, 2018

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
Copy link
Contributor

Nemo157 commented Oct 23, 2018

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

@dnsl48
Copy link
Contributor Author

dnsl48 commented Nov 24, 2018

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
Copy link
Contributor Author

dnsl48 commented Nov 25, 2018

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
Copy link
Member

cuviper commented Nov 29, 2018

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
Copy link
Contributor Author

dnsl48 commented Dec 2, 2018

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.

src/lib.rs Outdated Show resolved Hide resolved
@dnsl48
Copy link
Contributor Author

dnsl48 commented Jan 27, 2019

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.

src/lib.rs Show resolved Hide resolved
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can live with the flag for trailing zeros, thanks.

Rounding is an issue though.

Can you also run this through cargo fmt?

fn format_as_decimal() {
use super::{Ratio, Zero};

let data: [(u8, u8, &'static str, bool); 60] = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table can be excluded from formatting: #[cfg_attr(rustfmt, rustfmt_skip)]
(The newer #[rustfmt::skip] won't work with older rustc.)

(1u8, 9u8, "0.1111111111111111111111111111111111111111111111111111111111111111", false),
(1u8, 8u8, "0.125", true),
(1u8, 7u8, "0.1428571428571428571428571428571428571428571428571428571428571428", false),
(1u8, 6u8, "0.1666666666666666666666666666666666666666666666666666666666666666", false),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We ought to be rounding values -- "0.166...667"

... which I realize is harder, but I think it's important.

remainder = remainder - red_div_rem_diff.1;

(remainder, digit)
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block deserves a lot of comments, as it's much less obvious than the rest.


/// Writes the number into the output buffer in decimal representation.
/// No leading zeroes (except a single zero for the integer part)
/// Returns remainder of the division
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I meant to comment here too -- it's not really clear what this remainder represents. It's not just numer % denom, but rather numer % (denom * 10^precision), right? I'm not really sure what anyone would do with that, except check that it's non-zero as you have in tests. That gets hairier if we start rounding too...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No wait, it must be (numer * 10^precision) % denom, right? See, this is not obvious... :)

@dnsl48 dnsl48 closed this Mar 23, 2019
@dnsl48 dnsl48 deleted the feature/as_decimal branch March 23, 2019 23:00
@dnsl48
Copy link
Contributor Author

dnsl48 commented Mar 23, 2019

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)
    }

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.

4 participants