Add round-tripping Equality/IComparable contract generation #838

lipchev · 2020-09-21T00:00:05Z

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

- 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 · 2020-09-21T00:12:54Z

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%)`	⬆️
...nitsNet/GeneratedCode/Quantities/Acceleration.g.cs	`82.60% <100.00%> (+3.15%)`	⬆️
...et/GeneratedCode/Quantities/AmountOfSubstance.g.cs	`82.93% <100.00%> (+3.09%)`	⬆️
...tsNet/GeneratedCode/Quantities/AmplitudeRatio.g.cs	`77.85% <100.00%> (+3.61%)`	⬆️
UnitsNet/GeneratedCode/Quantities/Angle.g.cs	`82.06% <100.00%> (+2.88%)`	⬆️
...tsNet/GeneratedCode/Quantities/ApparentEnergy.g.cs	`77.31% <100.00%> (+3.70%)`	⬆️
...itsNet/GeneratedCode/Quantities/ApparentPower.g.cs	`77.85% <100.00%> (+3.61%)`	⬆️
UnitsNet/GeneratedCode/Quantities/Area.g.cs	`84.51% <100.00%> (+1.22%)`	⬆️
UnitsNet/GeneratedCode/Quantities/AreaDensity.g.cs	`75.09% <100.00%> (+5.74%)`	⬆️
.../GeneratedCode/Quantities/AreaMomentOfInertia.g.cs	`79.48% <100.00%> (+3.75%)`	⬆️
... 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 · 2020-09-21T19:03:16Z

CodeGen/Generators/UnitsNetGen/QuantityGenerator.cs

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


I don't fully grok this one.
I tried reading up on he last comments in #717, but I still don't quite see why this works and whether it is a good idea or if it has some edge-cases to it. Could you try to ELI5 ?

I get that we are doing an average of comparing A and B in both A's unit and in B's unit.

What exactly does this solve? Is it when A's and B's units are really different and trying to represent either quantity in the other's unit results in big rounding errors? Why does averaging help?

Sure- so basically the HashCode is used in some collections for fast lookup of objects. It is part of the 'Equality Contract' - which states that 'Equals(x,y) => Equals(x.GetHashCode(), y.GetHashCode()))', yet at the same time the result of GetHashCode is not considered as uniquely identifying (it is possible to have the same HashCode for two objects for which Equals(x,y) == false). When creating hashing functions one tries to make the result as unique as possible- but not such that it violates the first condition- typically we take the result of combining the hashes of the properties that make up the Equals function.
In practice all collections that use the hash function by definition compare both the result of the hash comparison, followed by a full Equals check- the first condition skipping 99.99% of the non-equal entities, while the second one ensures that we are not being tricked by a hash-collision.

Not sure why we wouldn't want this compared to raw double comparison in equivalent units.

angularsen · 2020-09-21T19:05:16Z

CodeGen/Generators/UnitsNetGen/QuantityGenerator.cs

@@ -792,7 +794,8 @@ public bool Equals({_quantity.Name} other, double tolerance, ComparisonType comp
        /// <returns>A hash code for the current {_quantity.Name}.</returns>
        public override int GetHashCode()
        {{
-            return new {{ QuantityType, Value, Unit }}.GetHashCode();
+            var roundedBaseValue = Math.Round(GetValueInBaseUnit(), 5);
+            return new {{ QuantityType, roundedBaseValue }}.GetHashCode();


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?

QuantityType was added as you could have two different units (say Length and Area) with identical values. If they also had the same unit enum value (treated as int), they would have the same hash code.

Is it problematic that 1 kilogram and 1 meter shares hash code though? They would still not be equal.
As long two equal values produce the same hash code, we should be fine. There is no requirement that in-equal values should produce different hash codes, as mentioned in #838 (comment).

I believe we could add back the QuantityType though and keep the new rounded value, but I'm just trying to understand what the impact of dropping quantity type would be. Less performant lookup of quantities in an array due to hash code collisions?

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).

#717 (comment)

HashSet may have been a better example - for set operations. It too supports an IEqualityComparer to be passed in.

So one impact of not including quantity type in the hash code would be that

var hs = new HashSet<IQuantity>(); hs.Add(Mass.FromKilograms(1)); hs.Add(Mass.FromKilograms(1)); hs.Add(Length.FromMeters(1)); hs.Dump(); /* Today HashSet<IQuantity> (2 items) -- 1 kg 1 m */ /* This PR would change it to this? HashSet<IQuantity> (1 item) -- 1 m */

Ok I'm not sure what I'm reading but QuantityValue is still there hah. Ignore that.

Just for our own learning: it does work! I thought it would disallow it.
The difference is without the quantity type it has to call Equals to check if it's a real collision or not. Given how easy it is to avoid, without a significant hash cost, it should be included for performance.

By the way (I didn't know that) but renaming any of the variables forming the anonymous class new { QuantityType, roundedBaseValue }, would produce a wholly different result from the GetHashCode that follows it :)

Ok I'm not sure what I'm reading but QuantityValue is still there hah. Ignore that.

LOL! I read it that way too! 😆

angularsen · 2020-09-21T19:07:35Z