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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Jun 13, 2024

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 Added units for Astronomy. #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
Copy link
Collaborator Author

lipchev commented Jun 13, 2024

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
Copy link
Contributor

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

@lipchev
Copy link
Collaborator Author

lipchev commented Jul 6, 2024

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
Copy link
Collaborator Author

lipchev commented Jul 6, 2024

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
Copy link
Collaborator Author

lipchev commented Jul 6, 2024

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

  • Frequency.BUnit added in Add B Units #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 Add Tilt to Angle #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
Copy link
Collaborator Author

lipchev commented Jul 7, 2024

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

@angularsen angularsen added the pinned Issues that should not be auto-closed due to inactivity. label Jul 8, 2024
Comment on lines -160 to +165
"FromUnitToBaseFunc": "{x} / 6.852176556196105e-2",
"FromBaseToUnitFunc": "{x} * 6.852176556196105e-2",
"XmlDocSummary": "The slug (abbreviation slug) is a unit of mass that is accelerated by 1 ft/s² when a force of one pound (lbf) is exerted on it.",
"XmlDocRemarks": "http://en.wikipedia.org/wiki/Slug_(unit)",
"FromUnitToBaseFunc": "{x} * (32.17405 * 0.45359237)",
"FromBaseToUnitFunc": "{x} / (32.17405 * 0.45359237)",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I finally figured out the Slug- the coefficient in the referenced wiki page for (slug -> lb) : 32.17404 is in fact not an exact value, but should be given the expression 9.80665 / 0.3048 which evaluates to ~32.174048556430446194225721784777.
There is in fact a second article which first states that

the standard acceleration due to gravity, approximately 32.174049 ft/s2 (9.80665 m/s2)

but then finishes by:

This definition can be rephrased in terms of the slug. A slug has a mass of 32.174049 lb.

I'm no expert, but here's what I could find in the NIST publication from 2008 (Page 53):

22 The exact conversion factor is 4.535 923 7 E−01. All units in Secs. B.8 and B.9 that contain the pound refer to the avoirdupois
pound.
23 The exact conversion factor is 4.448 221 615 260 5 E+00 since the standard value of the acceleration due to gravity,
gn = 9.806 65 m/s 2 exactly, is used to define the kilogram-force: 1 kgf = 9.806 65 E+00 N exactly

From this I can deduce that the expression for the Impulse for both PoundForceSecond and SlugFootPerSecond should be {x} * 0.45359237 * 9.80665.
The same replacement should be done for Density and MassConcentration where the Slug is being referenced, but given how inaccurate the approximation was, I'm not going to bother doing it for v5 as it looks likely to break more tests..

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great effort, I love how you dig into the details of this 👏
Let me know if there is anything I can help with.

As for the above questions about removing problematic units, I'd prefer to not remove anything just for the reason that it does not fit the new exact conversion implementation. I'd rather add special cases for units that do things like Math.Sqrt, Math.Sin() and have them work like today.

If we however find units that are plain wrong or reasonably proven not in use, we can discuss removing those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Issues that should not be auto-closed due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants