-
Notifications
You must be signed in to change notification settings - Fork 383
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
Incorrect standard gravity/unit conversion constant #1306
Comments
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.
|
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 I was looking to write some tests but wasn't sure if they just go in the
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);
}
} |
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).
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 |
Use
If you plan on making several changes, please create multiple pull requests.
Yes, custom (manually written) tests go in Just see where how other tests are placed and try to follow that.
It says at least 7 significant figures, no upper limit. 👍
Only 3 quantities use
If you search, you'll find tons of previous discussions on Some discussion on NaN/InF and double vs decimal: As it is now, I would personally favor moving everything over to Hope this helps, just let me know if you have any further questions! |
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.
I have and I was in the 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 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. |
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: The whole approach to error tolerance could definitely be improved, I am open to ideas. |
Fixes #1306 Corrects the conversion rates of the force quantity to be the precisely defined values, relative to standard gravity. Adds a unit test that verifies that the conversion rate from kilograms divided by the conversion rate from newtons equals standard gravity (`9.80665` m/s<sup>2</sup>).
Working in the force calibration industry, and needing to occasionally perform mass conversion, I don't agree on the values that are used for force,
and mass, unit conversions. While not technically the standard acceleration due to standard gravity (standard gravity), these unit conversion rates are derived from that value and are accepted as the standard unit conversion rate.I'm not sure where the constant originally came from, but the furthest back I could find it referenced in this repository was at the below linked file. This value has carried through significant overhauls to the library for the last decade without any documentation about where it came from. I propose that the value
9.80665002864
be changed to9.80665
as per the standard definition of acceleration due to standard gravity.https://en.wikipedia.org/wiki/Gravitational_acceleration
https://en.wikipedia.org/wiki/Standard_gravity
Original
UnitsNet/Src/UnitsNet/Constants.cs
Line 25 in 986d10b
Current
UnitsNet/Common/UnitDefinitions/Force.json
Line 35 in bf08093
It may be worth pointing out that the conversion rate for newtons to kilograms-force is also derived from standard gravity and would require the value here to be changed as well. Other conversion rates, and quantities, are impacted by this "constant" as well.
For reference, we use
4.4482216152605 N/lbf
as the conversion rate for pounds-force to newtons and0.45359237 kgf/lbf
as the conversion rate for pounds-force to kilograms-force. And to calculate the value of standard gravity, we take the conversion rate oflbf -> N
divided by the conversion rate oflbf -> kgf
. In the context of Units.NET, it should be the conversion rate ofN -> N
divided by the conversion rate ofN -> kgf
. This can be used as a unit test to verify the values aren't changed in the future.https://en.wikipedia.org/wiki/Kilogram-force
Edit: I originally included the mass equations because I thought I remembered them being wrong as well. After double checking today, it appears the mass conversions are correct, but the force conversions are not. The values look like they are the result of some rounding errors but I can't quite put my finger on what it might be.
The text was updated successfully, but these errors were encountered: