Temperature.Equals method uses the wrong Unit for the tolerance parameter

[park-jasper]

Describe the bug
Temperature.Equals(Temperature other, Temperature tolerance) does the wrong conversion, and should be Temperature.Equals(Temperature other, TemperatureDelta tolerance)

To Reproduce
var t1 = Temperature.FromDegreesCelsius(10);
var t2 = Temperature.FromDegreesCelsius(10);
var equals = t1.Equals(t2, tolerance: Temperature.Zero);

Expected behavior
equals should be true

Actual behavior
throws System.ArgumentOutOfRangeException "Tolerance must be greater than or equal to 0 (Parameter 'tolerance')"

Additional context
So the problem is that Temperature.Zero is converterd from TemperatureUnit.Kelvin to TemperatureUnit.DegreeCelsius because t1 was specified using Temperature.FromDegreesCelsius.
This uses the wrong conversion, the tolerance parameter should be of type TemperatureDelta which converts correctly.

To fix this I see two problems that have to be solved:

• The Generation of the Equals method is not done such that a special delta unit can be specified in the quantity json, so the schema would need to be adapted
• TemperatureDelta does not support the conversion of tolerance.As(this.Unit) because this.Unit is of type TemperatureUnit and not TemperatureDeltaUnit
  • To implement this Conversion as a custom method, one would need to match all Units between TemperatureUnit and TemperatureDeltaUnit. But TemperatureDeltaUnit has no equivalent for TemperatureUnit.SolarTemperature, which would have to be added

[tmilnthorp]

Indeed the conversion to C (-273.15) results in a negative absolute tolerance.

That said, it is working as expected by the current design. We could add an overload with TemperatureDelta, but I am not sure we should even maintain that unit long-term (@angularsen?).

In the meantime, you can use

var equals = t1.Equals(t2, tolerance: Temperature.FromDegreesCelsius(0));

or just

var equals = t1.Equals(t2);

[park-jasper]

Hi Tristan, thank you for the swift reply.

I must disagree with your suggested alternatives. passing Temperature.FromDegreesCelsius(0) would result in a wrong result if t1 was specified using e.g. kelvin. the caller of that method should not need to know which unit t1 has and have to specify the tolerance accordingly.
The second approach using .Equals without the tolerance parameter is marked obsolete because it cannot compare t1 and t2 properly if they have been defined using different units, and it should not be the responsibility of the caller of .Equals to ensure that both have the same unit.

[tmilnthorp]

Fair point on the first one.

I have been away from the code for a bit, and just checked Equals(). It used to do a conversion before doing "strict" equality, but now we are "ultra-strict" in that we do no conversion first and check both the value and unit exactly. Whether we should or shouldn't convert first up for debate, but you are correct that it now will not work for you. Sorry.

It seems without modifications to UnitsNet your only option would be

var equals = t1.Equals(t2.ToUnit(t1.Unit));

Which is not exactly elegant :)

[park-jasper]

The suggested approach would work for me. But I am also looking to prevent the pitfall of someone else using the recommended .Equals method and then getting the wrong result. In my case the ArgumentOutOfRangeException was hard to miss, but in another case one may be comparing temperatures and it returning true, when it should not.

I can see that this is a quirk that is unique to Temperature and TemperatureDelta where they do not have the same conversion, and thus is a problem that requires some specialized logic for just those cases. But I also believe that it should not stay the way it is because the method does not return the right results.

I was going to do a PR with a suggestion for a fix as well, but then realised the problem is a bit bigger than just changing a method's parameter from Temperature to TemperatureUnit and adding a bit of conversion logic.

[angularsen]

Closing this due to inactivity. We need a design proposal for how this should work, to move this forward.