Incorrect standard gravity/unit conversion constant

[MH-ZShearer]

Incorrect standard gravity/unit conversion constant

[angularsen]

Great writeup! It all sounds very reasonable to me. Likely the constant was found by an online converter or similar that sometimes are inaccurate. Would you be up for attempting a pull request to fix these? I'm happy to assist.

[MH-ZShearer]

Thanks, I'm working on my changes now. But I'm running into an issue with the generated code and the file encodings. Even though I re-save the files with the original encodings, they are apparently not saving, regardless of using VS or VSCode. I thought I saw a recent issue in this repository regarding the encodings, but I can't seem to find it again.

Is there any reason I shouldn't prefer using decimals? I expect to commit this change separately. I also want to add the grams and ponds base units, but I think it's valuable to keep the kilo- units separate as they are a primary unit of force and most documentation refers to the kilo- prefixed units.

I was looking to write some tests but wasn't sure if they just go in the UnitsNet.Tests\CustomCode directory or not. I also reviewed the information in the wiki about Adding a New Unit and wanted to see if it still applied to writing my tests as well.

• If the conversion is a simple * or / operation, then prefer * in FromBaseToUnit and / in FromUnitToBase. As an example, Length.Centimeter is defined as "FromBaseToUnit": "{x} * 100" and "FromUnitToBase": "{x} / 100", instead of {x} / 0.001 and {x} * 0.001.
• Prefer 1e3 and 1e-5 notation instead of 1000 and 0.00001

This is what I am familiar with regarding writing tests and wanted some feedback before I submitted a PR. I think it would look nicer to group the units based on base unit (US units grouped together, newtons grouped together, etc.). Completely open to feedback on this as well.

public class ForceTests : ForceTestsBase
{
    private const decimal StandardGravity = 9.80665m;
    private const decimal PoundsConversionRate = 4.4482216152605m;

    protected override bool SupportsSIUnitSystem => true;

    protected override double MicronewtonsInOneNewton => 1e6;
    protected override double MillinewtonsInOneNewton => 1e3;
    protected override double NewtonsInOneNewton => 1;
    protected override double DecanewtonsInOneNewton => 1E-1;
    protected override double KilonewtonsInOneNewton => 1E-3;
    protected override double MeganewtonsInOneNewton => 1E-6;

    protected override double DyneInOneNewton => 1e5;

    protected override double KilogramsForceInOneNewton => (double)(1 / StandardGravity);
    protected override double TonnesForceInOneNewton => (double)(1 / (StandardGravity * 1_000m));

    protected override double KiloPondsInOneNewton => (double)(1 / StandardGravity);

    protected override double PoundalsInOneNewton => 7.23301;

    protected override double OunceForceInOneNewton => (double)(1 / (PoundsConversionRate / 16m));
    protected override double PoundsForceInOneNewton => (double)(1 / PoundsConversionRate);
    protected override double KilopoundsForceInOneNewton => (double)(1 / (PoundsConversionRate * 1_000m));
    protected override double ShortTonsForceInOneNewton => (double)(1 / (PoundsConversionRate * 2_000m));

    // ...

    public void KilogramForceDividedByNewtonEqualsStandardGravity()
    {
        double duration = Force.FromKilogramsForce(1) / Force.FromNewtons(1);
        Assert.Equal((double)StandardGravity, duration);
    }
}

[MH-ZShearer]

After investigating a bit more, it looks like you do have rules regarding testing. I would still like some feedback about this prior to committing potentially unsatisfactory changes (since I already have an issue regarding the encoding).

4. Fix generated test stubs to resolve compile errors tests

• Override the missing abstract properties in the unit test class (ex: LengthTests.cs)
• Specify value as a constant, not a calculation, with preferably at least 7 significant figures where possible.
• Triple-check the number you write here, this is the most important piece as it verifies your conversion function in the .JSON file
• If possible, add a comment next to the value with an URL to where the value can be verified.

_{I do have some qualms about these arbitrary rules, mainly that it encourages potentially less accurate values due to the 7 significant figure recommendation. But I believe this should be a dedicated discussion.}

I also happened to stumble upon the reason practically no units are using the decimal data type. I believe this should also be a separate discussion. It could improve the precision for quantities that have had precision values set for it.

[angularsen]

Is there any reason I shouldn't prefer using decimals?

Use double, it is the standard value type in UnitsNet, decimal is only used by 3 quantities and not for very good reasons.

I also want to add the grams and ponds base units

If you plan on making several changes, please create multiple pull requests.
For example, fix constants in one PR. Add units in another PR.

I was looking to write some tests but wasn't sure if they just go in the UnitsNet.Tests\CustomCode directory or not.

Yes, custom (manually written) tests go in CustomCode dir.
Generated tests (we have those too), go in GeneratedCode dir and have .g.cs extension.

Just see where how other tests are placed and try to follow that.

it encourages potentially less accurate values due to the 7 significant figure recommendation

It says at least 7 significant figures, no upper limit. 👍

practically no units are using the decimal data type

Only 3 quantities use decimal; Power, Information, BitRate.
Power is about to change to double in #1195.

Information and BitRate may or may not stay on decimal. Not decided yet. There is a lot of complexity in the code base just to support decimal for 2 quantities, while 100+ quantities use double.

If you search, you'll find tons of previous discussions on double vs decimal.
Generics may be an option to support any numeric type, but it has its own downsides described here: https://github.com/angularsen/UnitsNet/wiki/Experimental:-Generic-Math-and-INumber

Some discussion on NaN/InF and double vs decimal:
#1268 (comment)

As it is now, I would personally favor moving everything over to double until we have a better way to support generics or some other approach to let the user choose double vs decimal.

Hope this helps, just let me know if you have any further questions!

[MH-ZShearer]

Thank you for your feedback. I have already made one pull request regarding these changes. I have at least one more that I want to do based on your feedback.

If you search, you'll find tons of previous discussions on double vs decimal.

I have and I was in the decimal camp due to our lab's emphasis on precision. However, after speaking with someone much smarter than myself in programming, it was stated that double should be more than sufficient for us and that it doesn't make sense to change the library. However, their recommendation was the same conclusion most others have come to here; using generics is the best option. Unfortunately, double specific values NaN and Inf definitely make that more challenging. And I'm not confident enough in my abilities to really assist in that matter.

The only thing that I still have concerns with is regarding the unit testing precision. It's stated that the tests pass as long as the value is within an epsilon of 0.00001. I believe this is the wrong approach and the testing system should prefer testing significant digits. This is a bit harder to implement, and may break, but should improve the confidence in the accuracy of this library for those that care more about accuracy than performance, for example.

I did briefly attempt to search for modifications to the testing process, but I was unable to find anything worthwhile regarding this. Was the epsilon chosen due to simplicity? I see there are a lot of tests (close to 30,000) and improved precision checking may increase the time it takes to perform all of the tests if most of them check significant digits instead of just an epsilon.

[angularsen]

Let's have the epsilon error as a separate discussion and potential pull request, so this PR can focus on fixing the conversion function constants for the units.

Epsilon constant was arbitrarily chosen and primarily used to check that conversion from small/large units and the chosen base unit and back again are within the tolerance. Example: LengthTestsBase.ConversionRoundTrip

The whole approach to error tolerance could definitely be improved, I am open to ideas.