Add round-tripping Equality/IComparable contract generation #838
Conversation
- 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 Report
@@ 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
Continue to review full report at Codecov.
|
| 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Not sure why we wouldn't want this compared to raw double comparison in equivalent units.
| @@ -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(); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this 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
*/There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Ok I'm not sure what I'm reading but QuantityValue is still there hah. Ignore that.
LOL! I read it that way too! 😆
| var secondMass = firstMass.ToUnit(MassUnit.Microgram); | ||
|
|
||
| Assert.Equal(0, firstMass.CompareTo(secondMass)); | ||
| Assert.Equal(0, secondMass.CompareTo(firstMass)); |
There was a problem hiding this comment.
This resulted in -1 and 0 before this PR, so that seems like an improvement.
There was a problem hiding this comment.
Before considering this failure of the implementation of the IComparable interface I was actually leaning more towards the Equals contract that also included the current Unit (without doing any conversions) - however coming up with rounding-error-proof (hopefully) implementation of the CompareTo tipped the scales for me in the other direction.
In any case- for the Equality contract both options are still on the table..
| var firstMass = Mass.FromGrams(0); | ||
| var secondMass = Mass.FromGrams(double.Epsilon); | ||
|
|
||
| Assert.Equal(firstMass.GetHashCode(), secondMass.GetHashCode()); |
There was a problem hiding this comment.
I believe you mentioned this earlier from the docs.
If two objects compare as equal, the GetHashCode() method for each object must return the same value. However, if two objects do not compare as equal, the GetHashCode() methods for the two objects do not have to return different values.
Here the values are not equal, but the hash codes are still allowed to be equal. Nice!
There was a problem hiding this comment.
They don't have to, but it can save a lot of equality checking.
double v1 = 0.0;
double v2 = double.Epsilon;
Console.WriteLine(v1.GetHashCode());
Console.WriteLine(v2.GetHashCode());0
1
|
|
||
| namespace UnitsNet.Tests | ||
| { | ||
| public class QuantityComparisonTests |
There was a problem hiding this comment.
The implementation made a whole lot more sense after I found these tests, thank you!
I still don't fully understand the implementation and whether there can be any issues with it, but I am way more comfortable after seeing the excellent work on these test cases 👍
|
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? |
fixing GetHashCode typos
| }} | ||
|
|
||
| /// <summary>Returns true if greater than.</summary> | ||
| public static bool operator >({_quantity.Name} left, {_quantity.Name} right) | ||
| {{ | ||
| return left.Value > right.GetValueAs(left.Unit); | ||
| return left.CompareTo(right) == 1; |
There was a problem hiding this comment.
I like this notation better
|
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. |
|
Hey guys, ignore this if you're busy- just pinging out to ask if there is something I should do to move this forward. |
|
I tried to refresh my memory by looking back at the comments.
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. |
angularsen
added
the
pinned
|
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 ( |
|
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 |
|
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). |
|
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: For me, that is the most intuitive. I know, equality won't work except for trivial examples like |
…her.As(Unit) - changed the GetHashCode generation scheme: depending on the QuantityType alone (the unit & value are ignored) - updated the two relevant tests (that were previously passing using mixed units)
|
So, I've reverted the 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). |
merging the recently generated entities
Addresses issues posed in #717