Open majewsky opened 1 month ago
Hi @majewsky, thanks for reporting this!
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:
GenericFraction<u64>
18446744073709551615
100000000000000000000
(which does not fit into u64)NaN
, since from cannot return Result/OptionIf you use either GenericFraction<u128>
or BigFration
, your value should get parsed successfully.
I suspected something like this. Would it then make sense to have this be an impl TryFrom
instead of impl From
?
I agree. We should cater try_from for more reliable conversions.
It could probably make sense to consider deprecating from
for unreliable conversions:
GenericFraction<u8>::from::<f64>()
is unreliableBigFraction::from::<f32>()
is reliableI 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.
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.
This test fails: