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

Implemened the INumber<Fraction> interface (and more..) #81

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lipchev
Copy link
Contributor

@lipchev lipchev commented Jul 4, 2024

@danm-de Sorry, I didn't realize what branch I was on, and pushed my commit to the branch of this PR.. I'll update the title and description, but if you prefer- I can split them later. Anyway, there aren't any stoppers for me here- the same fix can be applied on my side, until this is merged, so take your time.

Implementation of the INumber<Fraction> interface

  • introduced the ToDecimalSaturating method which saturates the output value to the decimal range [MinValue, MaxValue]
  • improved the accuracy (and performance) of the checked ToDecimal() when the terms are beyond the decimal range
  • fixed the possible loss of precision of the ToDouble() method when one of the terms is exceeding the range of the double (fixes Fraction.ToDouble() returns NaN for {2 * MaxValue, 2 * MaxValue} #82 )
  • updated the tests and added some extreme values the ConvertToNumberBenchmark (no change among the previously tested values)

Fraction.Round: Adding a Fraction.Round overload with the "normalize" parameter:

  • when set to true- it ensures that the result is normalized (which wasn't always the case before)
  • when set to false- it maintains the decimal denominator (and skipping the GCD reduction)
  • the DecimalNotationFormatter now uses the non-normalized version (which should in theory be more performant)
  • added the required tests in Method_Round
  • updated the DecimalNotationBenchmarkResults (not much of a difference as shown by the benchmarks in the next comment)

- when set to true- it ensures that the result is normalized (which wasn't always the case before)
- when set to false- it maintains the decimal denominator (and skipping the GCD reduction)
- the DecimalNotationFormatter now uses the non-normalized version (which is more performant)
- added the required tests in Method_Round
- updated the DecimalNotationBenchmarkResults
@lipchev
Copy link
Contributor Author

lipchev commented Jul 4, 2024

There wasn't actually much of a visible change on the DecimalNotationFormatterBenchmarks (to my surprise) - but I i expect the Round itself (which isn't benchmarked anywhere) would likely show the normal difference..
Here are some of the cases for which I know (for sure) that we're hitting the new round..

image

The rest of it looks like noise..

image

And the allocations have just one very small decrease...

image

- introduced the ToDecimalSaturating method which saturates the output value to the decimal range [MinValue, MaxValue]
- improved the accuracy (and performance) of the checked ToDecimal when the terms are beyond the decimal range
- fixed the possible loss of precision of the existing ToDouble when one of the terms is exceeding the range of the double (danm-de#82)
@lipchev lipchev changed the title Adding a Fraction.Round overload with the "normalize" parameter Implemened the INumber<Fraction> interface (and more..) Jul 17, 2024
Comment on lines +173 to +192
public void CreateSaturating_should_return_zero_or_NaN() {
double.CreateSaturating(Fraction.NaN).Should().Be(double.NaN);
Complex.CreateSaturating(Fraction.NaN).Should().Be(new Complex(double.NaN, 0));
decimal.CreateSaturating(Fraction.NaN).Should().Be(decimal.Zero);
float.CreateSaturating(Fraction.NaN).Should().Be(float.NaN);
Half.CreateSaturating(Fraction.NaN).Should().Be(Half.NaN);
BigInteger.CreateSaturating(Fraction.NaN).Should().Be(BigInteger.Zero);
Int128.CreateSaturating(Fraction.NaN).Should().Be(Int128.Zero);
UInt128.CreateSaturating(Fraction.NaN).Should().Be(UInt128.Zero);
long.CreateSaturating(Fraction.NaN).Should().Be(0);
ulong.CreateSaturating(Fraction.NaN).Should().Be(default);
int.CreateSaturating(Fraction.NaN).Should().Be(default);
uint.CreateSaturating(Fraction.NaN).Should().Be(default);
nint.CreateSaturating(Fraction.NaN).Should().Be(nint.Zero);
UIntPtr.CreateSaturating(Fraction.NaN).Should().Be(UIntPtr.Zero);
short.CreateSaturating(Fraction.NaN).Should().Be(default);
ushort.CreateSaturating(Fraction.NaN).Should().Be(default);
byte.CreateSaturating(Fraction.NaN).Should().Be(default);
sbyte.CreateSaturating(Fraction.NaN).Should().Be(default);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know what the reason is, but this is how this is implemented in BCL.. I first thought it was just in order to avoid throwing an exception, but later realized that BigInteger.CreateSaturating(double.PositiveInfinity) actually throws an OverflowException..

Comment on lines +744 to +790
[Test]
public void CreateSaturating_should_return_the_expected_value() {
var fraction = new Fraction(-3, 2);

double.CreateSaturating(fraction).Should().Be(-1.5);
Complex.CreateSaturating(fraction).Should().Be(new Complex(-1.5, 0));
decimal.CreateSaturating(fraction).Should().Be(-1.5m);
float.CreateSaturating(fraction).Should().Be(-1.5f);
Half.CreateSaturating(fraction).Should().Be((Half)(-1.5));
BigInteger.CreateSaturating(fraction).Should().Be(-1);
Int128.CreateSaturating(fraction).Should().Be(-1);
UInt128.CreateSaturating(fraction).Should().Be(0);
long.CreateSaturating(fraction).Should().Be(-1);
ulong.CreateSaturating(fraction).Should().Be(0);
int.CreateSaturating(fraction).Should().Be(-1);
uint.CreateSaturating(fraction).Should().Be(0);
nint.CreateSaturating(fraction).Should().Be(-1);
UIntPtr.CreateSaturating(fraction).Should().Be(0);
short.CreateSaturating(fraction).Should().Be(-1);
ushort.CreateSaturating(fraction).Should().Be(0);
byte.CreateSaturating(fraction).Should().Be(0);
sbyte.CreateSaturating(fraction).Should().Be(-1);
}

[Test]
public void CreateTruncating_should_return_the_expected_value() {
var fraction = new Fraction(-3, 2);

double.CreateTruncating(fraction).Should().Be(-1.5);
Complex.CreateTruncating(fraction).Should().Be(new Complex(-1.5, 0));
decimal.CreateTruncating(fraction).Should().Be(-1.5m);
float.CreateTruncating(fraction).Should().Be(-1.5f);
Half.CreateTruncating(fraction).Should().Be((Half)(-1.5));
BigInteger.CreateTruncating(fraction).Should().Be(-1);
Int128.CreateTruncating(fraction).Should().Be(-1);
UInt128.CreateTruncating(fraction).Should().Be(new UInt128(18446744073709551615, 18446744073709551615));
long.CreateTruncating(fraction).Should().Be(-1);
ulong.CreateTruncating(fraction).Should().Be(18446744073709551615UL);
int.CreateTruncating(fraction).Should().Be(-1);
uint.CreateTruncating(fraction).Should().Be(4294967295u);
nint.CreateTruncating(fraction).Should().Be(-1);
UIntPtr.CreateTruncating(fraction).Should().Be(new UIntPtr(18446744073709551615));
short.CreateTruncating(fraction).Should().Be(-1);
ushort.CreateTruncating(fraction).Should().Be(65535);
byte.CreateTruncating(fraction).Should().Be(255);
sbyte.CreateTruncating(fraction).Should().Be(-1);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another contentious point- currently all integer based types (including the unsigned versions) are created using an intermediate conversion to BigInteger:

  • TOther.CreateSaturating((BigInteger)fraction) // where TOther might be the "int" type
  • TOther.CreateTruncating((BigInteger)fraction)

The BigInteger has separate implementation for each of the TryConvertTo.. versions, with the truncating one, doing the overflowing as demonstrated by the second set of tests.
However, as I was looking at the sources for the decimal/double types, it seems they have just one implementation for the TryConvertToSaturating and TryConvertToTruncating (with the latter calling the former)..

I assume, whoever wants to create unsigned numbers from fractions knows what he's doing.. 🤷

Comment on lines +154 to +164
/// <summary>
/// Converts the fraction to a decimal number. If the fraction cannot be represented as a decimal due to its
/// size, the method will return the closest possible decimal representation (either decimal.MaxValue or
/// decimal.MinValue).
/// </summary>
/// <returns>
/// The decimal representation of the fraction. If the fraction is too large to represent as a decimal, returns
/// decimal.MaxValue. If the fraction is too small to represent as a decimal, returns decimal.MinValue.
/// </returns>
/// <remarks>If the fraction is NaN (the numerator and the denominator are both zero), the method will return decimal.Zero.</remarks>
public decimal ToDecimalSaturating() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course this one doesn't have to public, but I guess somebody might find some use for it (also, it was easier to test, rather than having to go through the INumber interface... 😄 ).

As a side note- a lot of the methods from INumber that are redundant with our existing properties (or are generally not interesting to the user) were implemented as explicit. Feel free to add or remove the methods that you feel would/ wouldn't be useful to the user (I'm talking about things like IsFinite(Fraction), IsInteger(Fraction) etc..).

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.

Fraction.ToDouble() returns NaN for {2 * MaxValue, 2 * MaxValue}
1 participant