UnitsNet: GetHashCode returns different hash for Equal units
Describe the bug
.GetHashCode()
returns different hash for 2 units for which .Equals()
return true
To Reproduce
Length inCm = Length.FromCentimeters(100);
Length inM = Length.FromMeters(1);
Assert.AreEqual(inCm, inM);
Assert.AreNotEqual(inCm.GetHashCode(), inM.GetHashCode());
Expected behavior
The GetHashCode()
should return the same value because the .Equals() returns true
Additional context From the remarks:
Two objects that are equal return hash codes that are equal. However, the reverse is not true: equal hash codes do not imply object equality, because different (unequal) objects can have identical hash codes.
I’ve peeked in the sources and I think the issue is, that the value from which the unit is constructed is used when calculating the hashcode:
Shouldn’t this be normalized to the unit’s base unit or something along those lines?
Reason I’m asking is because I’m doing IEquatable
implementations on my objects and in the .Equals(...)
method, I’m comparing Units to a max of 5 decimals while in the GetHashCode()
method I’m simply using the GetHashCode()
method from the unit.
During the PR someone pointed out that for the GetHashCode()
, the unit should also be rounded using the same logic as in the .Equals(...)
method:
/// <inheritdoc />
public bool Equals(ResultAnalysis1DInternalForces other)
{
if (ReferenceEquals(null, other))
{
return false;
}
if (ReferenceEquals(this, other))
{
return true;
}
return base.Equals(other)
&& Equals(Member, other.Member)
&& Equals(MemberRib, other.MemberRib)
&& Index == other.Index
&& Section.UnitsNetEquals(other.Section)
&& N.UnitsNetEquals(other.N)
&& Vy.UnitsNetEquals(other.Vy)
&& Vz.UnitsNetEquals(other.Vz)
&& Mx.UnitsNetEquals(other.Mx)
&& My.UnitsNetEquals(other.My)
&& Mz.UnitsNetEquals(other.Mz);
}
/// <inheritdoc />
public override bool Equals(object obj)
{
return ReferenceEquals(this, obj) || obj is ResultAnalysis1DInternalForces other && Equals(other);
}
/// <inheritdoc />
public override int GetHashCode()
{
unchecked
{
int hashCode = base.GetHashCode();
hashCode = (hashCode * 397) ^ (Member != null ? Member.GetHashCode() : 0);
hashCode = (hashCode * 397) ^ (MemberRib != null ? MemberRib.GetHashCode() : 0);
hashCode = (hashCode * 397) ^ Index;
hashCode = (hashCode * 397) ^ Section.GetHashCode();
hashCode = (hashCode * 397) ^ N.GetHashCode();
hashCode = (hashCode * 397) ^ Vy.GetHashCode();
hashCode = (hashCode * 397) ^ Vz.GetHashCode();
hashCode = (hashCode * 397) ^ Mx.GetHashCode();
hashCode = (hashCode * 397) ^ Mz.GetHashCode();
return hashCode;
}
}
public static bool UnitsNetEquals<TUnit>(this TUnit value, TUnit other)
{
if (value == null && other == null)
{
return true;
}
if (value != null && other != null && value is IQuantity thisUnit && other is IQuantity otherUnit)
{
try
{
double thisValue = thisUnit.Value;
double otherValueInThisUnits = otherUnit.As(thisUnit.Unit);
return Comparison.Equals(thisValue, otherValueInThisUnits, ComparingConstants.DoubleComparisionDelta, ComparisonType.Absolute);
}
catch (ArgumentException e)
{
return false;
}
}
return EqualityComparer<TUnit>.Default.Equals(value, other);
}
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Comments: 16 (10 by maintainers)
Commits related to this issue
- ✨ Add back IEquatable with strict equality Fixes #1017 Reverted removing IEquatable and changed the implementation to strict equality and improved the xmldocs a bit. Reverts commit f3c7e2559319b882... — committed to angularsen/UnitsNet by angularsen 2 years ago
- v5 Release (#982) Fixes #180 Merging the v5 release branch. It is still in alpha, but it is functional, nugets are published and there are not many planned breaking changes left. By merging, ... — committed to angularsen/UnitsNet by angularsen 2 years ago
- ✨ Add back IEquatable with strict equality Fixes #1017 Reverted removing IEquatable and changed the implementation to strict equality and improved the xmldocs a bit. Reverts commit f3c7e2559319b882... — committed to angularsen/UnitsNet by angularsen 2 years ago
- ✨ Add back IEquatable with strict equality Fixes #1017 Reverted removing IEquatable and changed the implementation to strict equality and improved the xmldocs a bit. Reverts commit f3c7e2559319b882... — committed to angularsen/UnitsNet by angularsen 2 years ago
- ✨ Add back IEquatable with strict equality Fixes #1017 Reverted removing IEquatable and changed the implementation to strict equality and improved the xmldocs a bit. Reverts commit f3c7e2559319b882... — committed to angularsen/UnitsNet by angularsen 2 years ago
- ✨ Add back IEquatable with strict equality (#1100) Fixes #1017 - Reverted removing `IEquatable`. - Changed the implementation to strict equality on both `Value` and `Unit`. - Improved tests. - ... — committed to angularsen/UnitsNet by angularsen 2 years ago
I think the approach that @dschuermans is using in the
ResultAnalysis1DInternalForces
is the correct one- I’m also doing this for some of my classes- like when comparing two “measurements” (e.g. from an analytical balance)- I know what the resolution is, so I can safely implement a value based equality contract (think about rounding to the nearest nanogram). In other cases I known that the relative difference between the expected and the target concentration should not be more than some % - so I implement it accordingly (typically in a IEqualityComparer of the given class).I don’t know if there is a reasonable default rounding precision that can be assumed when comparing using the default Equals/GetHashCode methods. By the way- I just recently discovered how Delphi 6 implements the
SameValue
function:However in #838 I didn’t actually go with the rounding approach- but was rather relying (at least initially) on the use of the IComparable interface - which is still not fixed (this test would fail in the current code-base).
This main problem with the IComparable approach was that it isn’t transitive - X.ToUnit(…).ToUnit(X.Unit) doesn’t necessarily convert back to X so- having a conversion function in the Equals method would seem like a bad idea.
As much as I’d like to have the proper value-based-unit-agnostic-equality-comparison-contract, it just doesn’t seem like it’s possible with the double arithmetic (I personally think that a
decimal
/rational
type would have been better suited for use with real quantities- but that’s another can of worms).As for fixing the Doubles- I think the best approach would be to have the strict equality contract (with an exception for Zero) and a Roslyn Analyzer that should generate a warning when ‘comparing floating point numbers’.
That sounds good to me!
Hi, will try to respond soon, need to digest it all first