surprising NaN in conversion from f64

[majewsky]

This test fails:

#[test]
fn just_a_normal_float_nothing_to_see() {
    let foo = fraction::Fraction::from(0.00003233568143917312);
    assert!(!foo.is_nan());
}

[dnsl48]

Hi @majewsky, thanks for reporting this!

[dnsl48]

Hi @majewsky,
That may be surprising because it is not too well documented.
However, this is expected behaviour for values that don't fit into the underlying types.
If you would like to handle big values gracefully, you may need to use BigFraction, which uses heap allocated memory for storing huge numbers.

Longer explanation:

• Fraction is GenericFraction<u64>
• u64::MAX is 18446744073709551615
• initial denominator is 100000000000000000000 (which does not fit into u64)
• int overflow is converted to NaN, since from cannot return Result/Option

If you use either GenericFraction<u128> or BigFration, your value should get parsed successfully.

[majewsky]

I suspected something like this. Would it then make sense to have this be an impl TryFrom instead of impl From?

[dnsl48]

I agree. We should cater try_from for more reliable conversions.
It could probably make sense to consider deprecating from for unreliable conversions:

• e.g. GenericFraction<u8>::from::<f64>() is unreliable
• while BigFraction::from::<f32>() is reliable

[majewsky]

I randomly came across how another similar library solves this problem, and will leave this here as potential inspiration.

The bigint/bigfloat implementation in the Go standard library provides a cast from floating-point numbers into Rat (which is short for "rational" and their version of the Fraction type), that does not panic on loss of accuracy. Instead, it reports in a second return value whether accuracy was lost: https://pkg.go.dev/math/big#Float.Rat

I don't know whether it is feasible to actually obtain the required information from the float-to-fraction conversion algorithm that you're using, but from an API design perspective, it would be nice to have, say:

impl TryFrom<f64> for GenericFraction<u64> {
    type Error = LossOfPrecisionError<GenericFraction<u64>>; // placeholder name
}

struct LossOfPrecisionError<F> {
    best_estimate: F,
    // ...other fields to describe the loss of precision...
    // (e.g. the Go library reports whether the estimate is below or above the true value)
}

Library users could then decide to .unwrap() the conversion result to replicate the existing behavior, or do something like .unwrap_or_else(|err| err.best_estimate) to continue with the best-effort value.

[dnsl48]

Thanks for the feedback.

I agree, that's one of the sensible ways to do that.
I feel we can do both - provide the standard API implementations such as try_from and from, AND we can also add the best estimate feature to the approx module.

The current from logic was implemented years ago before TryFrom trait was stabilised in the std library. Now it feels to be the right time to make full use of it.