Skip to content

MaxMagnitude(-2, 2) returns -2 and MinMagnitude(2, -2) returns 2, the opposite of the INumberBase contract #111

Description

@matt-edmondson

What's wrong

PreciseNumber.MaxMagnitude and MinMagnitude (PreciseNumber/PreciseNumber.cs, ~lines 1100-1108) break ties in favour of the first argument:

public static PreciseNumber MaxMagnitude(PreciseNumber x, PreciseNumber y) => x.Abs() >= y.Abs() ? x : y;
public static PreciseNumber MinMagnitude(PreciseNumber x, PreciseNumber y) => x.Abs() <= y.Abs() ? x : y;

The INumberBase<T> contract, as implemented by int, double, and decimal, says what happens on a tie:

  • MaxMagnitude returns the positive value.
  • MinMagnitude returns the negative value.

MaxMagnitudeNumber and MinMagnitudeNumber delegate to these methods, so they have the same problem.

Repro

Call PreciseNumber int / double
MaxMagnitude(-2, 2) -2 2
MinMagnitude(2, -2) 2 -2

The current tests (TestStaticMaxMagnitude and related, PreciseNumberTests.cs ~588-620) only use 1 and -1 in the order that hides the problem.

Why it matters

PreciseNumber is sold as a drop-in INumber<T>. Generic math written against INumberBase<T>, such as a norm, clamping, or picking a pivot, gives different results when T is PreciseNumber instead of double. The result also depends on argument order, so MaxMagnitude(a, b) != MaxMagnitude(b, a) for a = -b.

Suggested fix

public static PreciseNumber MaxMagnitude(PreciseNumber x, PreciseNumber y)
{
    int c = x.Abs().CompareTo(y.Abs());
    return c > 0 ? x : c < 0 ? y : IsNegative(x) ? y : x;
}
public static PreciseNumber MinMagnitude(PreciseNumber x, PreciseNumber y)
{
    int c = x.Abs().CompareTo(y.Abs());
    return c < 0 ? x : c > 0 ? y : IsNegative(x) ? x : y;
}

Acceptance criteria

  • MaxMagnitude(-2, 2) == 2 and MaxMagnitude(2, -2) == 2.
  • MinMagnitude(-2, 2) == -2 and MinMagnitude(2, -2) == -2.
  • Tests cover both argument orders for the tie case, for all four methods.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions