UnitsNet icon indicating copy to clipboard operation
UnitsNet copied to clipboard

v4.117.0 breaks quantity comparison between different units

Open gmkado opened this issue 11 months ago • 3 comments

Describe the bug

Comparison with a smaller unit (e.g. MillidegreesCelsius) fails when the larger unit (e.g. DegreesCelsius) is set using double.minValue or double.maxValue.

To Reproduce Steps to reproduce the behavior (just an example):

  1. Using dotnet fiddle, project type = console, compiler set to .NET 8
  2. Include UnitsNet package 4.116.0
  3. Run the code from below. Prints "False"
  4. Change to 4.117.0
  5. Run the code again, this time it throws System.ArgumentException: PositiveInfinity or NegativeInfinity is not a valid number. (Parameter 'value')
using System;
using UnitsNet;
public class Program
{
	public static void Main()
	{		
		var min = Temperature.FromDegreesCelsius(double.MinValue);
		var temperature = Temperature.FromMillidegreesCelsius(100);
		
		Console.WriteLine(temperature < min);
	}
}

Expected behavior I'd expect this not to throw and to just do the comparison as expected.

Looks like it fails because the comparison now attempts to convert double.MinValue degrees to millidegrees. In both versions, this will throw:

var min = Temperature.FromDegreesCelsius(double.MinValue);
var throws = min.ToUnit(TemperatureUnit.MillidegreeCelsius);

Additional context

We do this comparison when we're trying to keep a running min while iterating over a list. There are workarounds, like initializing min to the smallest unit we would possibly use or setting it to the first value in the list. I'm reporting this anyways as I think the comparison should still work, even if the use case isn't common.

Traced back to (I think) #1023

gmkado avatar Mar 04 '24 20:03 gmkado

Generally speaking, you should avoid combining double.Min/MaxValue with units, since you can easily run into overflows when converting to other smaller/larger units. Overflows for double results in Infinity/NegativeInfinity,

The problem is that any conversion in Units.NET typically goes via a base unit. You can define direct conversions between specific units, but it would require a lot of work to define direct conversions between all units for each quantity.

In your example, MinValue Celsius is converted to Kelvins by adding 273.15, Then Kelvin is converted to MillidegreeCelsius by first subtracting 273.15, getting back to MinValue, and then multiplying by 1000. MinValue is -1,7976931348623157E+308, and multiplying this by 1000 means you get double.NegativeInfinity since it becomes smaller than the smallest double value.

In essence: double.MinValue * 1000 = −∞

// UnitsNet\GeneratedCode\Quantities\Temperature.g.cs:845

                (TemperatureUnit.DegreeCelsius, TemperatureUnit.Kelvin) => new Temperature(_value + 273.15, TemperatureUnit.Kelvin),
                (TemperatureUnit.Kelvin, TemperatureUnit.MillidegreeCelsius) => new Temperature((_value - 273.15) * 1000, TemperatureUnit.MillidegreeCelsius),

This could be solved by UnitsNet automatically generating direct conversions between all prefix units, so it simply multiplies by 10 to go from Centi to Milli etc. instead of going via base unit. If you are interested in attempting a pull request, I'm happy to assist.

On a side note, in v6 we are moving towards allowing Nan/Infinity again since we are removing support for decimal type. I don't think it will help for this case though, since you still lose the actual value to Infinity. There is a prerelease nuget 6.0.0-pre003 you could try if you like.

In summary, don't use Min and Max values. If you are willing, it may be possible to improve conversion between prefixes.

angularsen avatar Mar 04 '24 20:03 angularsen

Actually, using double.Infinity is semantically closer to what I'm after. If I'm understanding correctly, in v6 we could do something like

var min = Temperature.FromDegreesCelsius(double.Infinity);

foreach(var temperature in myTemperatures) // myTemperatures units are unknown, could be millidegrees
{
    if(temperature < min) 
    {
        min = temperature;
    }
}

And the comparison wouldn't throw? If so I think we can close this one.

gmkado avatar Mar 06 '24 05:03 gmkado

Give 6.0.0-pre003 a try and see if it helps

angularsen avatar Mar 06 '24 05:03 angularsen

Closing as fixed in v6, where infinity values are once again supported.

angularsen avatar Jul 08 '24 13:07 angularsen