UnitsNet icon indicating copy to clipboard operation
UnitsNet copied to clipboard

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

Open lipchev opened this issue 1 year ago • 6 comments

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:

  • HeatFlux : due to the change in Calorie
  • HeatTransferCoefficient : due to the change in Calorie
  • Impulse : PoundFootPerSecond, SlugFootPerSecond were represented by a single coefficient that is significantly different from what got: I've used the expression for the Poundal: {x} * (0.45359237 * 0.3048) (0.138254954376) instead of {x} / 7.230657989877 (0.13830000000000150747000000001643)- although seeing how close this is to 0.1383 there might be something I'm missing here...
  • Luminosity : there are two conflicting values for the coefficient online- the top result on google points to this page (where the original value was taken from), while the linked article on Wikipedia (as well as the article about the Sun) both have it listed as 3.828×1026 W[5]. This was added in #680 by @ebfortin - hopefully he could confirm which one is the correct value.
  • Power: MechanicalHorsepower changed from {x} * 745.69 to {x} * 76.0402249 * 9.80665 as given by this article
  • Pressure the TechnicalAtmosphere coefficient (no idea where the old one was taken from)
  • SpecificFuelConsumption : I'm not a domain expert, but {x} * 28.33 doesn't look right for PoundMassPerPoundForceHour (lb/(lbf·h))- here's what I got: {x} * 453.59237 / (0.0044482216152605 * 3600)
  • ThermalResistance: using different conversion coefficients for BTU and Calorie from the one defined in Energy (note: I wasn't able to confirm the "correct" conversion coefficient for BTU).
  • VolumetricHeatCapacity: using different conversion coefficient for Calorie

lipchev avatar Jun 13 '24 15:06 lipchev

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.

lipchev avatar Jun 13 '24 20:06 lipchev

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

ebfortin avatar Jun 18 '24 11:06 ebfortin

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 avatar Jul 06 '24 00:07 lipchev

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 avatar Jul 06 '24 00:07 lipchev

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

  • Frequency.BUnit added in #904 is the only expression in the code-base that requires Math.Sqrt, which makes it non-reversible (doesn't work with my plan for v6)
  • Angle.Tilt added in #918 is the only expression requiring the use of Math.Sin and Math.Asin, which again won't work in both directions..

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 avatar Jul 06 '24 02:07 lipchev

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...

lipchev avatar Jul 07 '24 00:07 lipchev

fixed in release/v6

lipchev avatar Apr 11 '25 09:04 lipchev