Convert Ratio of integers to f64

[MattX]

Performs the division with integer arithmetic, and rounds
before performing an exact cast to float. This avoids any
issues with rounding.

General method referenced from https://stackoverflow.com/questions/33623875/converting-an-arbitrary-precision-rational-number-ocaml-zarith-to-an-approxim.

Partially fixes #4 (no support for f32 conversion currently).

[maxbla]

Try running ci/test_full.sh before pushing. This will test your code very similarly to how Travis CI tests it. We should probably add this instruction to the readme, or a contributing.md.

It seems like your code depends on std. This crate supports nostd. If you can make it work with FloatCore, that would be great, but it seems like that isn't possible with your algorithm. You might be able to use integer_decode, min_value and min_positive_value to make this work with FloatCore.

If you want CI to work, try throwing in some #[cfg(feature = "std")]s. After all, if this implementation is specific to f32 and f64s its going to need std.

[MattX]

Ok, I'll make sure that passes.

Given that this code needs BigInts, and num-bigint seems to have a hard dependency on std, does it make sense to try and convert it to FloatCore?

[cuviper]

You need #[cfg(feature = "bigint")] at least. If you're using any of f64's methods that aren't in core, then we should explicitly require "std" too, as num-bigint may someday work with only core and alloc. FloatCore is only relevant if you're using generic Float, but I think not.

[MattX]

Ok that makes sense, I'll add both feature flags.

I think most of what I use is actually in core, except for exp2. It could potentially be replaced by a hand-rolled solution, but I don't think it's really worth it until we actually get std-free BigInts.

[MattX]

@maxbla I agree with your comments above, I've pushed a new revision which handles both 16-bit architectures and arbitrarily wide usizes. Thanks for having taken the time to look at this!

[MattX]

@cuviper, @hauleth, it looks like you guys have merge rights on this repo, would you have time to give this a look?

[MattX]

I've moved a bigint-based unit test to the doctests to show the possibility of using BigInts. Does anyone have time to take another look at the review?

[cuviper]

Sorry, yes, I mean to give this a deeper review, but haven't gotten to it yet. It just came up in #54 too -- is there a reason you added a new method instead of implementing ToPrimitive as a whole?

[MattX]

No particular reason—it's more work to implement all of ToPrimitive, I guess 😛. This code can easily enough be extended to f32s (a particularly trivial implementation being to cast the f64 down), but if possible I'd rather merge this first.

I do have a small reason for not doing the full ToPrimitive, which is that it's not clear to me what the exact semantics of ToPrimitive are. Should None be returned only when the value overflows? Or when it's not exactly representable? We should probably document this in the trait documentation.

[cuviper]

This code can easily enough be extended to f32s (a particularly trivial implementation being to cast the f64 down)

FWIW, the default ToPrimitive::to_f32 already does this. The only required methods are to_i64 and to_u64, and the rest should only be implemented if you can do a better job than the naive default. (e.g. a more precise to_f64 as here, or to_u128 that actually uses more bits)

but if possible I'd rather merge this first.

The design of public API is important though, because we'll be stuck with it. I think ToPrimitive is probably the better approach. Maybe we can do both (as discussed here), but if so I'd want to make sure their use is a close as possible, without weird idiosyncrasies between the two options. The surest way to do that is to implement one in terms of the other.

I do have a small reason for not doing the full ToPrimitive, which is that it's not clear to me what the exact semantics of ToPrimitive are. Should None be returned only when the value overflows? Or when it's not exactly representable? We should probably document this in the trait documentation.

Sure, we can and should enhance docs -- the current semantics of the primitive implementations are that they only return None for overflow, or for completely unrepresentable values like NaN to int. Float to integer returns the same value that an as cast would, truncated toward zero.

[MattX]

Ok, that makes sense. I'll put out a new version that implements ToPrimitive.

[MattX]

I've pushed a version with ToPrimitive, please let me know what you think.

[MattX]

@cuviper, @hauleth, let me know how the CR looks when you have time. I'm happy to make further changes if needed.

[maxbla]

If the original author doesn't makes the suggested changes, I'd be happy to drag this PR to the finish line.

[cuviper]

@maxbla that's fine with me, just keep the original commit for authorship and then add your own. It needs to be rebased too.

[MattX]

Hey sorry, I think I missed the review notification. @maxbla if you have time feel free to take over, otherwise I can take a look this weekend.

[maxbla]

I would be happy for you to make the suggested changes. I only offered because it looked like you were done.

[MattX]

Yeah if you have time to get started before this weekend go for it!

[MattX]

I've implemented the suggested changes:

• If big ints and std are available, use that to convert any Ratio of Clone + Integer + ToPrimitive + ToBigInt types
• Otherwise, ratios of primitive integer types ≤ 32 bits are now always convertible to f64 by converting the numerator and denominator to f64 directly. Ratios of u64 and i64 are convertible if std is available using the same algorithm as BigInt conversion, and using i128 as the supporting type for shifts.

[cuviper]

Looks great, thanks!

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push