search

angularsen / UnitsNet

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

Decomposed the rounded expressions in the unit definitions into operations with exact coefficients #1393

Open lipchev opened 5 months ago

lipchev commented 5 months ago

Motivation

Many of the expressions used in the UnitDefnitions (json files) currently correspond to the result of the division of two numbers that do not produce a terminating decimal: see how in #1389 the conversion for VolumeFlow for UkGallonsPerHour) is given as 791887.667.

There are of course many cases where the resulting decimal is simply rounded, or is the result of some online converter that is introducing similar rounding errors (I don't know any that operate using rational numbers).

What is worse is that the AI agents are also picking up on these incorrect results, sometimes referencing this library.

Working with the double type, up until now, these rounding errors were not very important- however as per my upcoming proposal for v6 (incorporating the Fractions as the underlying value type), there wouldn't be any rounding errors.

Approach

I've taken the following approach when working through the list:

  1. Start with the most basic quantities: Mass / Length etc - look at every coefficient, and the corresponding Wikipedia page- find where it says exactly equal to ... and take that (putting the corresponding link in the xml-docs).
  2. If the Wikipedia article says something like 1/16 of something-else I use expression-for-something-else / 16, where the expression-for-something-else is either a coefficient or another expression.
  3. Move to Area then to Volume etc., working in the exact same way- taking coefficients or expressions that are either exactly equal to ... or (more often) referencing one of the previously verified files

Result

This resulted in almost half of all quantities receiving some modification to their expressions. Most of these are related to conversions involving the US/British units but there is also some confusion regarding the Calorie - the value of 4.1868 was used (instead of 4.184) in a few places, which is a note-worthy difference.

Here are all the tests that broke:

lipchev commented 5 months ago

I wasn't able validate the conversion expressions for Pressure head - from what I've read it seems like it involves an additional parameter (density / specific weight): https://www.mydatabook.org/fluid-mechanics/pumps/head-to-pressure-converter/

I haven't checked, but if I were to try to decompose the expression in Pressure.json- I'd probably find the ~density of water somewhere in there. No sure if this is correct or not- I'm just saying that I've skipped these two units (MeterOfHead and FootOfHead).

The same goes for the MetersOfElevation- I'm pretty sure that's not a valid Unit.. I think we should make it into a method / property for v6.

ebfortin commented 5 months ago

For Luminosity 3.828x10^26 would be the correct one since it comes from NASA.

lipchev commented 4 months ago

I also think that for v6 we should replace the PressureUnit.FeetOfElevation and PressureUnit.MeterOfElevation with properties that convert to/from Length:

Here's what I have in mind:

    /// <summary>
    ///     Calculates the pressure at a given elevation.
    /// </summary>
    /// <param name="elevation">The elevation for which to calculate the pressure.</param>
    /// <param name="significantDigits">The number of significant digits to use in the calculation. Default is 13.</param>
    /// <returns>A Pressure struct representing the pressure at the given elevation.</returns>
    /// <remarks>
    ///     The calculation is based on the formula for pressure altitude from Wikipedia:
    ///     https://en.wikipedia.org/wiki/Pressure_altitude
    /// </remarks>
    public static Pressure FromElevation(Length elevation, int significantDigits = 13)

    /// <summary>
    ///     Converts the pressure to an equivalent elevation or altitude.
    /// </summary>
    /// <param name="significantDigits">The number of significant digits to round the result to. Default is 15.</param>
    /// <returns>A <see cref="Length" /> object representing the equivalent elevation or altitude.</returns>
    /// <remarks>
    ///     The conversion is based on the formula for pressure altitude as described on Wikipedia
    ///     (https://en.wikipedia.org/wiki/Pressure_altitude).
    /// </remarks>
    public Length ToElevation(int significantDigits = 15)

There is unfortunately no way to make the conversion exact:

    [Fact]
    public void PressureFromElevation_ConvertsWithRounding()
    {
        var pressureFromElevation = Pressure.FromElevation(new Length(129149.9769457631m, LengthUnit.Foot), 13);
        Assert.Equal(1, pressureFromElevation.Pascals);
    }

    [Fact]
    public void ElevationFromPressure_ConvertsWithRounding()
    {
        Length elevationFromPressure = Pressure.FromPascals(1).ToElevation(16);
        Assert.Equal(39364.91297306859288m, elevationFromPressure.Meters);
    }
lipchev commented 4 months ago

I would also suggest that for the FuelEfficiency we replace the BaseUnit (LiterPer100Kilometers) with KilometerPerLiter as this way FuelEfficiency.Zero would bring you exactly 0 km closer to your destination.. 🚀

PS Technically, I'd say that LiterPer100Kilometers should be another quantity altogether (I'd call it FuelConsumption) but that's not very important, as long as we support NaN and Infinity..

lipchev commented 4 months ago

I also have an issue with the following two "units":

I was going to suggest replacement functions for constructing with / converting to a number (e.g. Frequency.FromBUnit(..)) but I fail to find any information online about either of these units, so I wonder if those methods are actually useful..

@bplett-wgtcorp Are you still relying on these units? Do you think there are still active applications that depend on them?

lipchev commented 4 months ago

I wasn't able validate the conversion expressions for Pressure head - from what I've read it seems like it involves an additional parameter (density / specific weight): https://www.mydatabook.org/fluid-mechanics/pumps/head-to-pressure-converter/

I haven't checked, but if I were to try to decompose the expression in Pressure.json- I'd probably find the ~density of water somewhere in there. No sure if this is correct or not- I'm just saying that I've skipped these two units (MeterOfHead and FootOfHead).

I'm still not 100% that either of the values is correct but I am pretty confident that they can't both be right:

Assuming that MetersOfHead -> Pascals is {x} * 9804.139432 (which is what this calculator uses), then we would expect that FeetOfHead would be {x} * 9804.139432 * 0.3048 or simply {x} * 2988.3016988736 but what we have is {x} * 2989.0669 (which is not what this calculator uses).

Fixing it would most certainly break a test...