Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v4.117.0 breaks quantity comparison between different units #1376

Open
gmkado opened this issue Mar 4, 2024 · 3 comments
Open

v4.117.0 breaks quantity comparison between different units #1376

gmkado opened this issue Mar 4, 2024 · 3 comments
Labels

Comments

@gmkado
Copy link

gmkado commented Mar 4, 2024

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 gmkado added the bug label Mar 4, 2024
@angularsen
Copy link
Owner

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.

@gmkado
Copy link
Author

gmkado commented Mar 6, 2024

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.

@angularsen
Copy link
Owner

angularsen commented Mar 6, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants