danm-de / Fractions

A fraction data type to calculate with rational numbers.
BSD 2-Clause "Simplified" License
49 stars 15 forks source link

Fraction.ToDouble() returns NaN for {2 * MaxValue, 2 * MaxValue} #82

Open lipchev opened 2 months ago

lipchev commented 2 months ago

Let's illustrate this with an example:

new Fraction((BigInteger)double.MaxValue * 2, (BigInteger)double.MaxValue * 2, false)

<!DOCTYPE html>

  | Name | Value | Type -- | -- | -- | -- ▶ | A | 3.59538627e+308 | System.Numerics.BigInteger ▶ | B | 3.59538627e+308 | System.Numerics.BigInteger   | State | Unknown | Fractions.FractionState ▶ | StringFormats | "1" | Fractions.Fraction.FractionDebugView.StringFormatsView ◢ | ValueFormats | NaN | Fractions.Fraction.FractionDebugView.NumericFormatsView   | Decimal | 1 | decimal   | Double | NaN | double   | Integer | 1 | int   | Long | 1 | long

I actually discovered the issue when one of my square roots turned up with a 0:

        var largeNumber = BigInteger.Pow(10, 309);
        var fraction = new Fraction(4 * largeNumber, largeNumber, false);
        var actual = fraction.Sqrt();  // is Fraction.Zero..

..but I would have likely caught it when testing the INumber interface and the TryConvertToChecked, TryConvertToSaturating and TryConvertToTruncating (which are the only functions that require special attention).

I've already started implementing the INumber<Fraction> interface (two weeks ago) but wanted to finish up the tests for my (still unpublished) PR for UnitsNet - when one of the tests failed (note this wasn't some static test concocted with an extreme value, it was an actual root-sum-of-squares that failed one of the tests in my own code-base)..

lipchev commented 1 week ago

note this wasn't some static test concocted with an extreme value, it was an actual root-sum-of-squares that failed one of the tests in my own code-base

I wanted to shed some light into the reason why I was getting such huge numbers- the reason is in fact with the way that the Sqrt function is implemented- although the iteration breaks as soon as the specified accuracy is achieved, this doesn't mean that the resulting fraction hasn't got hundreds of digits in its terms... Now, if you feed those hundred digits a second time (after squaring them for good measure), guess how many you digits you get back... In about ten iterations I got to see a number with over 500 000 digits (the value evaluated to ~0.53).

The simple fix on my side, was to add an explicit Round to the end of my Sqrt extension.. not sure if this should be the default, but I think we should at least add a remark/warning..

By the way there is also a possible performance optimization that I must have missed (flipping the while(..) into a do..while(..)):

public static Fraction Sqrt(this Fraction x, int accuracy = 30) {
    //Babylonian Method of computing square roots

    if (accuracy <= 0) {
        throw new ArgumentOutOfRangeException(
            paramName: nameof(accuracy),
            actualValue: accuracy,
            message: string.Format(Resources.AccuracyIsLessThanOrEqualToZero, accuracy));
    }

    if (x.IsNaN || x.IsNegative) {
        return Fraction.NaN;
    }

    if (x.Numerator.IsZero || x.Denominator.IsZero) // (x.IsZero || x.IsInfinity)
    {
        return x;
    }

    var tolerance = new Fraction(BigInteger.One, BigInteger.Pow(new BigInteger(10), accuracy));

    //Using Math.Sqrt to get a good starting guess
    var guessDouble = Math.Sqrt((double)x);
    var oldGuess = double.IsInfinity(guessDouble)
        ? x / Fraction.Two
        : (Fraction)guessDouble;

    Fraction newGuess;
    do {
        //Babylonian Method
        newGuess = (oldGuess + (x / oldGuess)) / Fraction.Two;
        oldGuess = newGuess;
    } while ((oldGuess - newGuess).Abs() > tolerance);

    return newGuess;
}

PS This is also likely the reason why my implementation of the nth root in #32 had such terrible performance..