Skip to content

Array.Sort / OrderBy put SignificantNumber values in the wrong order: IComparable compares at reduced significance, which isn't transitive #105

Description

@matt-edmondson

What's wrong

CompareTo(SignificantNumber) (SignificantNumber.cs:706) and CompareTo(object) (:721) call the static CompareTo(PreciseNumber, PreciseNumber) (:363). That method rounds both values to the lower of their two significant-digit counts before comparing. The README says so on purpose ("compare both numbers at the lower of their significant digit counts"). The problem is that this is also the IComparable / IComparable<SignificantNumber> implementation, and that is what Array.Sort, List.Sort, OrderBy, SortedSet and Comparer<T>.Default use. Those all assume a consistent total order, and this comparison is not one:

  • 1.23.CompareTo(1.2) returns 0, and 1.2.CompareTo(1.17) returns 0, but 1.23.CompareTo(1.17) returns 1. That isn't transitive.
  • It also disagrees with the rest of the type. For the same pair, 1.23 > 1.2 is true, 1.23 == 1.2 and Equals are false (both compare exact values, and ==/Equals come from the record struct), and the generic CompareTo<T> overloads (:736, :748) return 1.

Reproduced on .NET 10 with the repo's SignificantNumber.cs against ktsu.PreciseNumber 2.6.2:

Array.Sort / OrderBy(x => x) on [1.17, 1.23, 1.2, 1.21, 1.19]
actual:   1.17, 1.23, 1.2, 1.19, 1.21
expected: 1.17, 1.19, 1.2, 1.21, 1.23

Why it matters

Sorting a list of measurements is ordinary use, and here it gives a wrong order with no error. With a non-transitive comparer, SortedSet/SortedDictionary can also end up with broken invariants: lookups miss, and two values the operators call different are treated as duplicates.

Suggested fix

Make the interface implementations (IComparable<SignificantNumber>.CompareTo and IComparable.CompareTo) compare exact values, the same way the operators, Equals and CompareTo<T> already do. Keep the significance-aware comparison as an explicitly named API, for example CompareAtSignificance(left, right) or an IComparer<SignificantNumber> named for that behaviour, and update the README. Any comparison that rounds to the lower precision of each pair can't be a total order, so it shouldn't be the default comparer.

Acceptance criteria

  • Array.Sort of the example above gives 1.17, 1.19, 1.2, 1.21, 1.23.
  • For every pair, a.CompareTo(b) == 0 if and only if a == b, and the sign of a.CompareTo(b) agrees with < / >.
  • The significance-aware comparison is still available under its new name, with tests.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    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