Add round-tripping Equality/IComparable contract generation

[lipchev]

Addresses issues posed in #717

• CompareTo implemented in terms of the two-way comparison between 'this.As(other)' & 'other.As(this)'
• Equals changed to CompareTo(other) == 0
• Comparison operators re-written in terms of CompareTo values
• GetHashCode generation modified such that it is consistent with the Equals function- i.e. not including the UnitType
• Added tests (QuantityComparisonTests) for the IComparable, Equality contracts, including the (previously failing) comparisons with problematic values such as 0.001

[codecov-commenter]

Codecov Report

Merging #838 (28d55bc) into master (d89306b) will increase coverage by 2.96%. The diff coverage is 99.60%.

@@            Coverage Diff             @@
##           master     #838      +/-   ##
==========================================
+ Coverage   80.45%   83.42%   +2.96%     
==========================================
  Files         285      285              
  Lines       42607    43876    +1269     
==========================================
+ Hits        34281    36603    +2322     
+ Misses       8326     7273    -1053     

Impacted Files	Coverage Δ
...et/GeneratedCode/Quantities/MassConcentration.g.cs	89.14% <20.00%> (+1.73%)	:arrow_up:
...nitsNet/GeneratedCode/Quantities/Acceleration.g.cs	82.60% <100.00%> (+3.15%)	:arrow_up:
...et/GeneratedCode/Quantities/AmountOfSubstance.g.cs	82.93% <100.00%> (+3.09%)	:arrow_up:
...tsNet/GeneratedCode/Quantities/AmplitudeRatio.g.cs	77.85% <100.00%> (+3.61%)	:arrow_up:
UnitsNet/GeneratedCode/Quantities/Angle.g.cs	82.06% <100.00%> (+2.88%)	:arrow_up:
...tsNet/GeneratedCode/Quantities/ApparentEnergy.g.cs	77.31% <100.00%> (+3.70%)	:arrow_up:
...itsNet/GeneratedCode/Quantities/ApparentPower.g.cs	77.85% <100.00%> (+3.61%)	:arrow_up:
UnitsNet/GeneratedCode/Quantities/Area.g.cs	84.51% <100.00%> (+1.22%)	:arrow_up:
UnitsNet/GeneratedCode/Quantities/AreaDensity.g.cs	75.09% <100.00%> (+5.74%)	:arrow_up:
.../GeneratedCode/Quantities/AreaMomentOfInertia.g.cs	79.48% <100.00%> (+3.75%)	:arrow_up:
... and 200 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a7c52a8...b2ba83f. Read the comment docs.

[angularsen]

I think this looks pretty good. Just some loose questions/comments above.

@tmilnthorp you were involved in #717 too, maybe take a look when you have the time?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[lipchev]

Hey guys, ignore this if you're busy- just pinging out to ask if there is something I should do to move this forward.

[angularsen]

I tried to refresh my memory by looking back at the comments. I think for me, the only think I'm still unsure about is the hard coded constant of 5. Can we run into any issues by this choice? Also add some code comments to that part.

https://github.com/angularsen/UnitsNet/pull/838#discussion_r492282925

This deserves some comments explaining the rationale for the GetHashCode. It is not intuitive to me, had I not known about the discussion. Why is 5 a good choice here?

Honestly- it's an over-estimation of the max rounding error that we could get by converting to base (it's so stated in the Precision chapter of the wiki)

I updated the precision wiki article a bit to be a bit more accurate with today's code base, but the essence remains as before. https://github.com/angularsen/UnitsNet/wiki/Precision

[lipchev]

I gave it some more thought tonight- and I no longer think this is a good idea. Not that it doesn't do what it was supposed to (q == q.ToUnit(X)) but it just doesn't make any sense to actually use it- given the limiting constraint that q.ToUnit(X).ToUnit(Y) is not necessarily equal to q (the precision lost on the way is non-recoverable). Any method with a quantity as input cannot thus be allowed to make any sane assumptions of equality beyond "both unit and value match" (you'd just be one more ToUnit() away from ruining your day). I therefore propose that we revert to using the GetValueInBaseUnit() for the Comparator and the strict Equals/HashCode equality contract (using both Unit & Value)

[angularsen]

I agree it sounds like the safest option. I still don't feel this equality is all that useful though, for any non-trivial unit conversion there will be loss of precision and two very similar quantities will still be considered in-equal. And even worse, it is an implementation detail where some quantities use decimal.

I think custom equality comparison methods adds more value than trying to fit the quantities into IEquatable and IComparable, but if we should offer an implementation I would vote for the most conservative/safest option.

[lipchev]

The problem is that the "safest" option in this case {unit, value} would be quite a breaking change. Anyone who has ever used something like x == Mass.Zero or default(Mass) could be in trouble if 'x' does not come with the default unit (kg in this case). Also there are some edge cases that might come as a surprise: like having X<=Y & X >= Y but X != Y.

[angularsen]

Good point. Changing equality from base unit to value+unit would break things like Zero comparison, I hadn't thought of that. So. Hm.

Actually. I think we should honor this contract, not value+unit:

Length.FromCentimeters(0).Equals(Length.Zero)

For me, that is the most intuitive. I know, equality won't work except for trivial examples like Length.FromCentimeters(100).Equals(Length.FromMeters(1)), but I just don't see a better compromise given this very limited contract.

[lipchev]

So, I've reverted the Equals method (same as before) and made the GetHashCode() return only the QuantityType's hash-code- as such it would match the contract requirements. However, I'm pretty sure there is 'reflexivity' requirement that the Equals method does not currently satisfy (or at least not always). I've adjusted the mixed-unit tests such as to demonstrate the problematic behavior.

As far as the Comparable interface- there I've kept the proposed scheme (that is reflexive). I think this is the safer option, even if there could be cases where A == B && A.CompareTo(B) != 0, the inverse is always true, i.e. A != B => A.CompareTo(B) != 0.

All in all, I'm not terribly happy with the whole thing- it would have been so much neater with a decimal-like value representation. In any case, even if we kept the original implementation- it wouldn't hurt to grab the tests from this branch (and make a note about the limitations in the wiki).