Possible issues with the IComparable interface and associated operators

[lipchev]

Hey guys, I've mentioned this issue back in #838 where the current implementation of the CompareTo is prone to the same rounding issues as with the old equality contract. Here's the test that I expect would still fail in the current code base:

    [Fact]
    public void CompareTo_WithSameQuantityInAnotherUnit_ReturnsZero()
    {
        var firstMass = Mass.FromGrams(0.001);
        var secondMass = firstMass.ToUnit(MassUnit.Microgram);

        Assert.Equal(0, firstMass.CompareTo(secondMass));
        Assert.Equal(0, secondMass.CompareTo(firstMass));
    }

This might not be an issue when sorting a collection (although it might not be obvious why sorting [x1, x2, x3] might yield a different ordering as compared to the result of sorting [x2, x1, x3]), however I'm quite worried about the > operator.

Here's the typical use-case that may be problematic: if (mass > Capacity) // possible rounding error here

I'm starting think that my proposal for handling the result of both x1.As(x2.Unit).CompareTo(x2.Value) and x2.As(x1.Unit).CompareTo(x1.Value) might be "better" than what we have right now (covering 99% of the use cases) however, there is still the possibility of getting an unexpected result from Capacity.ToUnit(x).ToUnit(y) > Capacity..

I don't know, the stricter alternative is simply too painful to imagine..

PS There is still the option of trying to use rational numbers for the underlying type: now that there is only one interface to implement this could probably be hidden from the client (say we store the fraction inside the QuantityValue). There is probably just the issue with serializing a rational using the single-value contract (some precision maybe lost), however we could roll out a RationSerializationSerializer for those that need it.. Obviously this wouldn't be as "lightweight" as what we have right now, and there is also the possible issues with the external dependency (e.g. compatibility with the older/compact frameworks etc)..

[angularsen]

My head hurts 😅

Yes there is a potential rounding error on all these:

• IComparison.CompareTo()
• < > comparison operators
• Equals(quantity, tolerance)

However, I believe this only happens if the units are different.

I don't immediately see how we can avoid this though. Any ideas?

I'm starting think that my proposal for handling the result of both x1.As(x2.Unit).CompareTo(x2.Value) and x2.As(x1.Unit).CompareTo(x1.Value) might be "better" than what we have right now

Could you elaborate? Do you mean to take an average of the two results?

There is still the option of trying to use rational numbers for the underlying type: now that there is only one interface to implement this could probably be hidden from the client (say we store the fraction inside the QuantityValue)

Rational numbers can probably help in the cases where rational numbers can be defined, but I guess it can't solve all our units? In v6 the QuantityValue has now been removed by the way. It's all double now.

I've redirected #1368 in here too, as it seems to touch the same topic.

[lipchev]

var asFirstUnit = other.GetValueAs(this.Unit);
var asSecondUnit = GetValueAs(other.Unit);
return (_value.CompareTo(asFirstUnit) - other.Value.CompareTo(asSecondUnit)) / 2;

The idea was that the rounding error may only occur in the one direction so that (hopefully) at least of the CompareTo operations has it right.

As for the rationals - I don't see why using something like this wouldn't work (as long as we keep the fractions normalized - we could have the exact equality contract back). The only downside I see is the performance hit (and the extra dependency, should we decide the take it/expose it). I think there is already a fork of UnitsNet that went that route, but I cannot seem to find it right now.. Soo, I don't know how you feel about it- it's been ages since I wanted to try this approach- been eying the removal of the IDecimalQuantity, but must have missed that QuantityValue is going as well.. Have you planned a release date for v6?

[angularsen]

No release date for v6 yet, some work still remains and there are already pretty big changes in there, so I plan to keep prerelease versions out for some time so we can gather a bit of feedback.

Fraction nuget is new to me, interesting. I have not tried it nor BigInteger, but it does sound like it maybe will incur a performance penalty to represent all numbers and perform arithmetic using rational numbers.

I'd be interested to test it out and get a better feel of the tradeoffs, but in essence, it would be like replacing double with a super version of decimal with a lot more bits to represent fixed precision, right? There would still be a limit to the precision, just a lot higher?

[lipchev]

I'm not sure there is a limit to the BigInterger- i think you can put as many bits as you like..

[lipchev]

Ok, just a heads up- I've started work on "re-fractioning" the v6 here. I'll create a PR as soon as I figure out the Math.Sin({x}..

[angularsen]

Cool, keep me posted on how it works out 🙌

[lipchev]

@angularsen This is looking very good (personally I'm 100% convinced):

	Name	Value	Type
	(Mass.FromGrams(1) / 3).ToUnit(MassUnit.SolarMass).ToUnit(MassUnit.Gram) * 3 == Mass.FromGrams(1)	true	bool

	Name	Value	Type
	BitRate.FromBytesPerSecond(1).As(BitRateUnit.BitPerSecond) == 8	true	bool

And here are the benchmark results (the conversion methods are just about 10x slower- which I think is a very small price to pay):

1. Before:

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19045
AMD Ryzen 9 7900X, 1 CPU, 24 logical and 12 physical cores
.NET SDK=8.0.200
  [Host]     : .NET 6.0.27 (6.0.2724.6912), X64 RyuJIT
  DefaultJob : .NET 6.0.27 (6.0.2724.6912), X64 RyuJIT

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
Constructor	4.561 ns	0.0372 ns	0.0348 ns	-	-	-
Constructor_SI	211.041 ns	1.8334 ns	1.7150 ns	0.0114	-	192 B
FromMethod	12.953 ns	0.1071 ns	0.1002 ns	-	-	-
ToProperty	26.804 ns	0.2964 ns	0.2772 ns	-	-	-
As	26.983 ns	0.2244 ns	0.1989 ns	-	-	-
As_SI	211.884 ns	2.5722 ns	2.4061 ns	0.0114	-	192 B
ToUnit	24.616 ns	0.3022 ns	0.2679 ns	-	-	-
ToUnit_SI	211.295 ns	1.8718 ns	1.7509 ns	0.0114	-	192 B
ToStringTest	712.395 ns	7.7697 ns	7.2678 ns	0.0448	-	760 B
Parse	49,103.580 ns	804.3356 ns	752.3761 ns	4.0894	0.1221	69,245 B
TryParseValid	47,693.856 ns	355.5437 ns	296.8951 ns	4.0894	0.1831	69,238 B
TryParseInvalid	35,195.649 ns	472.2180 ns	441.7130 ns	3.1738	0.1831	54,003 B
QuantityFrom	27.279 ns	0.1479 ns	0.1235 ns	0.0033	-	56 B
IQuantity_As	28.635 ns	0.2681 ns	0.2508 ns	0.0014	-	24 B
IQuantity_As_SI	210.206 ns	2.4136 ns	2.2576 ns	0.0114	-	192 B
IQuantity_ToUnit	26.915 ns	0.4073 ns	0.3810 ns	0.0033	-	56 B
IQuantity_ToStringTest	721.960 ns	7.6593 ns	7.1645 ns	0.0448	-	760 B

After:

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19045
AMD Ryzen 9 7900X, 1 CPU, 24 logical and 12 physical cores
.NET SDK=8.0.200
  [Host]     : .NET 6.0.27 (6.0.2724.6912), X64 RyuJIT
  DefaultJob : .NET 6.0.27 (6.0.2724.6912), X64 RyuJIT

Method	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
Constructor	12.46 ns	0.130 ns	0.122 ns	-	-	-
Constructor_SI	230.78 ns	3.937 ns	3.683 ns	0.0114	-	192 B
FromMethod	12.75 ns	0.133 ns	0.124 ns	-	-	-
ToProperty	318.87 ns	3.288 ns	3.075 ns	-	-	-
As	317.32 ns	3.353 ns	3.136 ns	-	-	-
As_SI	212.07 ns	2.185 ns	2.044 ns	0.0114	-	192 B
ToUnit	310.23 ns	3.109 ns	2.908 ns	-	-	-
ToUnit_SI	469.49 ns	3.715 ns	3.293 ns	0.0114	-	192 B
ToStringTest	726.00 ns	4.699 ns	3.924 ns	0.0467	-	792 B
Parse	49,045.47 ns	768.965 ns	719.290 ns	4.1504	0.1221	69,485 B
TryParseValid	49,352.21 ns	787.202 ns	736.349 ns	4.1504	0.1831	69,478 B
TryParseInvalid	34,432.10 ns	180.113 ns	140.620 ns	3.1738	0.1831	54,035 B
QuantityFrom	55.98 ns	0.716 ns	0.670 ns	0.0052	-	88 B
IQuantity_As	321.34 ns	3.456 ns	3.232 ns	0.0014	-	24 B
IQuantity_As_SI	219.85 ns	1.307 ns	1.223 ns	0.0114	-	192 B
IQuantity_ToUnit	319.19 ns	3.556 ns	3.326 ns	0.0052	-	88 B
IQuantity_ToStringTest	756.37 ns	9.583 ns	8.495 ns	0.0467	-	792 B

And here is a snippet of the generated code:

                // MassUnit -> BaseUnit
                (MassUnit.Centigram, MassUnit.Kilogram) => new Mass(_value / 100000, MassUnit.Kilogram),
                (MassUnit.Decagram, MassUnit.Kilogram) => new Mass(_value / 100, MassUnit.Kilogram),
                (MassUnit.Decigram, MassUnit.Kilogram) => new Mass(_value / 10000, MassUnit.Kilogram),
                (MassUnit.EarthMass, MassUnit.Kilogram) => new Mass(_value * 597220 * BigInteger.Pow(10, 19), MassUnit.Kilogram),

I'll review my changes later (or more likely tomorrow) and open the PR. The tests still do not compile (haven't even started updating them yet) but I think we have enough to get us started on the discussion of the future role (if any) of the doubles, the QuantityType and the use of Fraction for the interfaces..

[lipchev]

PS In the second benchmark I'm returning Fractions instead of doubles (As/ToProperty), so the comparison isn't exactly fair- we'd have to possibly include the cost of converting from a Fraction to double (but that's another benchmark- which we should offer to the original library, with our best regards and so on..)

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions[bot]]

This issue was automatically closed due to inactivity.