.Minimum and .Maximum are mixed up

[mschmitz2]

Test code:

var p1 = Pressure.FromBar(10);
var p2 = Pressure.FromBar(20);

Console.WriteLine($"{nameof(p1)}: {p1}"); // prints "p1: 10 bar"
Console.WriteLine($"{nameof(p2)}: {p2}"); // prints "p2: 20 bar"
Console.WriteLine($"p1.Minimum(p2): {p1.Minimum(p2)}"); // prints "p1.Minimum(p2): 20 bar"
Console.WriteLine($"p2.Minimum(p1): {p2.Minimum(p1)}"); // prints "p2.Minimum(p1): 20 bar"
Console.WriteLine($"p1.Maximum(p2): {p1.Maximum(p2)}"); // prints "p1.Maximum(p2): 10 bar"
Console.WriteLine($"p2.Maximum(p1): {p2.Maximum(p1)}"); // prints "p2.Maximum(p1): 10 bar"

[mschmitz2]

See also the implementation of System.Math.Min() and System.Math.Max(). Might want to handle this in the same or a similar way.

public static double Min(double val1, double val2)
{
    // This matches the IEEE 754:2019 `minimum` function
    //
    // It propagates NaN inputs back to the caller and
    // otherwise returns the lesser of the inputs. It
    // treats +0 as greater than -0 as per the specification.

    if (val1 != val2)
    {
        if (!double.IsNaN(val1))
        {
            return val1 < val2 ? val1 : val2;
        }

        return val1;
    }

    return double.IsNegative(val1) ? val1 : val2;
}

public static double Max(double val1, double val2)
{
    // This matches the IEEE 754:2019 `maximum` function
    //
    // It propagates NaN inputs back to the caller and
    // otherwise returns the greater of the inputs. It
    // treats +0 as greater than -0 as per the specification.

    if (val1 != val2)
    {
        if (!double.IsNaN(val1))
        {
            return val2 < val1 ? val1 : val2;
        }

        return val1;
    }

    return double.IsNegative(val2) ? val1 : val2;
}

[MadsKirkFoged]

I totally get why you would use that method that way!.. And I might have to change the wording about it!

The meaning of Minimum is: ".CantGoBelow(100W)" The meaning of Maximum is: ".CantGoAbove(100W)"

ex.

var h1 = new Enthalpy(10, EnthalpyUnit.JoulePerKilogram);
var h2 = new Enthalpy(100, EnthalpyUnit.JoulePerKilogram);
var m1 = new MassFlow(1, MassFlowUnit.KilogramPerSecond);

Power p1 = (m1 * (h2 - h1)).Minimum(Power.Zero);

Maybe I should have called it UpperLimit/LowerLimit - What do you think?

[MadsKirkFoged]

What you are looking for is:

        MassFlow Min2 = UnitMath.Min(MassFlow.FromKilogramPerSecond(1),
                                     MassFlow.FromKilogramPerSecond(2));

[mschmitz2]

Oh well that explains things... and yes, of course UnitMath.Min is what I need. I remember stumbling upon UnitMath before and knowing it exists but must have forgotten about it and was using this monstrosity for a while: Pressure pMin = Pressure.FromSI(Math.Min(p1.SI, p2.SI));.

I actually only recently discovered the .Minimum method (as it's an extension method it gets suggested by the IDE when you type) and thought something along the lines of "a bit weird writing it like this but still better than what I was using before". Knowing now what you meant this to be this makes a lot of sense to me.

I agree that renaming to .UpperLimit()/.LowerLimit() would make this a lot clearer.

Alternatively, I thought a method .LimitTo() would be quite clear, but I guess in this you would have to make it take both a min and a max as it is otherwise not clear which limit is meant, e.g.

public static UnknownUnit LimitTo(this BaseUnit unit, UnknownUnit lowerLimit, UnknownUnit upperLimit)
{
    // with current naming
    // return unit.Minimum(lowerLimit).Maximum(upperLimit);

    // with new (?) naming
    return unit.LowerLimit(lowerLimit).UpperLimit(upperLimit);
}

Is there a way to include xml ///<summary></summary> with NuGet? That might have prevented me from doing this mistake as well (or at the very least prevented me from opening this issue once I looked at the source code).

Anyway, I'm happy with whatever you decide to do here. Using UnitMath solves this for me. Feel free to close the issue.

[mschmitz2]

System.Math calls methods equivalent to my LimitTo suggestion above Clamp, so maybe that's what it should be called for consistency? Though those are not extension methods, so maybe ClampTo makes sense for your extension?

// Code from System.Math
public static double Clamp(double value, double min, double max)
{
    if (min > max)
    {
        ThrowMinMaxException(min, max);
    }

    if (value < min)
    {
        return min;
    }
    else if (value > max)
    {
        return max;
    }

    return value;
}

[mschmitz2]

Edit: just noticed there is already Extensions.InRangeOf, which does exactly this. So I guess that the issue here really comes down to deciding whether is makes sense to rename things, for clarity or consistency with e.g. System.Math. I guess what threw me and possibly anybody used to System.Math off sometimes is that methods equivalent to System.Math methods are distributed between UnitMath and Extensions. That is not to say that this doesn't make sense since, you know, they are extensions, but this can make things a little confusing. Ran into this kind of issue in my own code a couple of times so I know there is no ideal solution.

[MadsKirkFoged]

I agree that keeping as close to System.Math as possible does make good sense. I'll go though System.Math and try to map all methods

[mschmitz2]

Nice job clarifying this (in addition to all the other nice improvements!) with the recent updates!