Closed MH-ZShearer closed 1 year ago
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 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*
inFromBaseToUnit
and/
inFromUnitToBase
. 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
and1e-5
notation instead of1000
and0.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);
}
}
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.
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: https://github.com/angularsen/UnitsNet/issues/1268#issuecomment-1630856242
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!
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
vsdecimal
.
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.
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.
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
https://github.com/angularsen/UnitsNet/blob/986d10b9d49cdfc97555e543dcf8e5843ef688e3/Src/UnitsNet/Constants.cs#L25
Current
https://github.com/angularsen/UnitsNet/blob/bf080938604ee6d59a6c8e27e25887861c1e9199/Common/UnitDefinitions/Force.json#L35
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.