angularsen / UnitsNet

Makes life working with units of measurement just a little bit better.
https://www.nuget.org/packages/UnitsNet/
MIT No Attribution
2.18k stars 379 forks source link

QuantityInfo.MinValue and QuantityInfo.MaxValue are missing #848

Closed bitbonk closed 3 years ago

bitbonk commented 3 years ago

Is your feature request related to a problem? Please describe. For a generic (WPF) UI where you can input any type of quantity, we need the ability to configure the maximum and minimum bounds that the user is currently allowed to enter for a given value. If the user can enter any value (unbounded), the bounds should be set to the minimum and maximum that the given quantity type generally allows. Unfortunately there is no generic way to determine the minimum and maximum of a given quantity type. Although all quantities have the concrete properties like Pressure.MinValue and Pressure.MaxValue, there is no generic/polymorph way to determine these values without knowing the quantity type at compile time.

Describe the solution you'd like Introduce the properties QuantityInfo.MinValue and QuantityInfo.MaxValue, just like you have already introduced the properety QuantityInfo.Zero:

    /// <summary>
    ///     Minimum value of quantity, such as <see cref="P:UnitsNet.Length.MinValue" />.
    /// </summary>
    public IQuantity MinValue { get; }

    /// <summary>
    ///     Maximum value of quantity, such as <see cref="P:UnitsNet.Length.MaxValue" />.
    /// </summary>
    public IQuantity MaxValue { get; }

Describe alternatives you've considered We are currently using the following code:

(IQuantity)quantityInfo.ValueType.GetProperty("MinValue").GetValue(null)
angularsen commented 3 years ago

I think this would be useful. Would you be interested in attempting a pull request? I'm happy to assist.

angularsen commented 3 years ago

A few notes:

  1. Some quantities use decimal, but most use double as their internal value. This implementation should reflect that.
  2. A quantity is a pair of value and unit. As an example, Length units span from Nanometer to Kilometer. So its MinValue should probably be double.MinValue with unit Nanometer and MaxValue be double.MaxValue with unit Kilometer.
  3. Generally, you can run into overflow issues when converting say double.MaxValue of unit Kilometer to unit Nanometer, but since MinValue chooses the smallest unit and MaxValue chooses the largest unit we should be safe from introducing this problem here, I think.
rohahn commented 3 years ago

It was rather simple to expose the MinValue and MaxValue from the specific quantity classes through the QuantityInfo class.

But unfortunately I noticed that the current min/max values are not very useful since they always use the base unit. They neither represent a physical limit (e.g. light speed or absolute zero temperature) nor a lossless conversion limit of the datatype or the largest value that can be expressed by Units.Net.

Regarding your notes:

Since the MinValue and MaxValue properties are of type IQuantity, currently the value is only available as double (without cast to the specific type or reflection). But since the range of double is larger than decimal, this should not be a problem. I assume precision is not really important for this range check.

Unless there are units where negative values are not allowed, I think both MinValue and MaxValue should use the smallest unit of the quantity (e.g. LenghtUnit.Nanometer) to ensure the value can be converted to any unit without reaching infinity. The smallest unit could either be calculated once for each quantity or stored in the .json file.

public static Length MaxValue { get; } = new Length(double.MaxValue, GetSmallestUnit());
public static Length MinValue { get; } = new Length(double.MinValue, GetSmallestUnit());

private static LengthUnit GetSmallestUnit()
{
    var b = new Length(1, BaseUnit);
    return Enum.GetValues(typeof(LengthUnit))
        .Cast<LengthUnit>()
        .Where(u => u != LengthUnit.Undefined)
        .OrderByDescending(u => b.ToUnit(u).Value)
        .First();
}

But this depends of course on the intention of the MaxValue property. If MaxValue would represent the largest value that can be expressed (but not converted) we would have to use the largest unit instead. Then we could introduce additional properties like SafeMaxValue and SafeMinValue.

I can create a PR if we can agree on an implementation.

angularsen commented 3 years ago

I assume precision is not really important for this range check.

I agree.

I think both MinValue and MaxValue should use the smallest unit of the quantity (e.g. LenghtUnit.Nanometer)

Will this not cause problems?

Length.MinValue.ToUnit(LengthUnit.Kilometer) // overflow

I realize now that my own proposal of MinValue with min unit and MaxValue with max unit would have the same issue.

converted to any unit without reaching infinity

Different units have different conversion functions, so I think this is difficult to guarantee anyway. For instance, Temperature has a linear function and Level has a logarithmic function.

Then we could introduce additional properties like SafeMaxValue and SafeMinValue.

How did you envision these to behave?

rohahn commented 3 years ago

@bitbonk and me use MaxValue and MinValue in a recipe editor, which should cope with arbitrary units, as defaults of a range validation if no custom range was specified.

It is highly unlikely that any of our users will ever enter a value near the minimum or maximum and convert it to a different unit. So when using MaxValue we only want to express that a value is "unbounded". The unit and convertibility of these values is more of a theoretical problem.

To solve our issue (avoid the usage of reflection to access MaxValue/MinValue), I could create a PR which just exposes these properties in QuantityInfo without changing their values at all.

But I would like to understand why MaxValue and MinValue were added to the library and how they are usually used.

There is also a potential problem with the decimal quantities. If you convert decimal.MaxValue to double and back to decimal an OverflowException is thrown. The rounding error in this conversion is about 1e13.

IQuantity q = Power.MaxValue;
var foo = new Power((decimal)q.Value, PowerUnit.Watt); // OverflowException

Since we only compare the values using IComparable, this is currently not a problem for us.

angularsen commented 3 years ago

But I would like to understand why MaxValue and MinValue were added to the library and how they are usually used.

I think it was probably added without sufficient thought. I have never used it myself, and the more I think about it the more it seems like maybe we should not offer Min/Max for quantities. It is not intuitive what values or units to expect and you risk running into overflow issues.

The benefit of keeping it is, that it encapsulates the complexity of what max value to safely choose for double vs decimal, and what unit to choose. Since we already have the properties and if you truly would make use of them in QuantityInfo, then I'm fine adding them to QuantityInfo and returning the values as-is.

The benefit of using BaseUnit as in the current implementation of Min/MaxValue is that it is less prone to run into overflow issues when comparing to other quantities, since it doesn't need to convert from a small/large unit to the base unit first.

tmilnthorp commented 3 years ago

I would argue Min/Max should be removed entirely :)

lipchev commented 3 years ago

I have also struggled with the Min/Max in the past (before realizing the overflow issue was making them impossible to work with). However I do think they could be quite useful - mostly as defaults for range filters and value coercion (when we don't want to mess with nullable values). The basic requirement is "comparability". The main issue is the overflow in ToUnit- and more precisely the runtime exception that is thrown in the constructor value checking.

  1. An easy way of achieving comparability would be to accept the infinities (and possibly NaN as well).
  2. The other option would be to use a type of safe Min/Max as suggested- but in this case we might also have to consider adding Epsilon values as well..

In summary, if we decide to keep the NumberGuard- I think there should be a safe way of checking input values such as to avoid the ArgumentException (imagine a method that is expected to divide one quantity by another- what sane check can one perform on the denominator? Using denominator > Zero is of course not 100% safe)

lipchev commented 3 years ago

There is also the issue of what do you do when the denominator is Zero- what reasonable result could be returned from such a method (other than the naturally resulting Nan/Infinity)? I am currently stuck with a bunch of nasty-pre-method-call-checks, for some calculated values- where the result could have simply been NaN/Infinity (imagine having a class with something like CalculatedDensity => WeightAdded / VolumeAdded starting with Zero/Zero and filling up from there).

bitbonk commented 3 years ago

mostly as defaults for range filters and value coercion (when we don't want to mess with nullable values)

I tend to think it would be much cleaner and less error prone to introduce a dedicated construct (e.g. Nullable or similar) in your user code that deliberately expresses that a min or max coercion is not set, rather than using a quantity's MinValue or MaxValue for it. So I would agree with @tmilnthorp to remove any Min/Max values from Units.NET altogether.

As it currently stands, MinValue and MaxValue would cause more confusion and create pitfalls than it would help.

angularsen commented 3 years ago

There is a use case for range checks, but I too would argue that this is best solved by the application. Either pick a small/large value or use nullability to identify unbounded min/max values.

I think we more or less agree to remove Min/Max from the library then? I created a PR to deprecate them.

https://github.com/angularsen/UnitsNet/pull/884

stale[bot] commented 3 years ago

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.