Add Angstrom unit #1042
Add Angstrom unit #1042
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1042 +/- ##
========================================
+ Coverage 81.5% 81.5% +0.1%
========================================
Files 308 308
Lines 48232 48241 +9
========================================
+ Hits 39284 39293 +9
Misses 8948 8948
Continue to review full report at Codecov.
|
Common/UnitDefinitions/Length.json
Outdated
| { | ||
| "SingularName": "Angstrom", | ||
| "PluralName": "Angstroms", | ||
| "FromUnitToBaseFunc": "{x}/1e-10", |
There was a problem hiding this comment.
@angularsen I have been using spaces in the functions to match the styling of the rest of the code. Would you agree that the following is more consistent?
"FromUnitToBaseFunc": "{x} / 1e-10",
"FromBaseToUnitFunc": "{x} * 1e-10",There was a problem hiding this comment.
Also, these are backwards. An Angstrom is 10e-10m. To go from Angstroms to meters, you must multiply.
"FromUnitToBaseFunc": "{x} * 10e-10",
"FromBaseToUnitFunc": "{x} / 10e-10",There was a problem hiding this comment.
I'm agree with you, your example is more consistent.
Also I didn't the follow the style guideline, other units are spaced like your example in the rest of the file.
There was a problem hiding this comment.
Yeah I would have to do a find/replace on all the json files to make it consistent with the rest of the code and generated code.
For example the generated prefixes have a lot of inconsistency:
unitConverter.SetConversionFunction<Length>(LengthUnit.Meter, LengthUnit.Nanometer, quantity => new Length((quantity.Value) / 1e-9d, LengthUnit.Nanometer));
unitConverter.SetConversionFunction<Length>(LengthUnit.Meter, LengthUnit.NauticalMile, quantity => new Length(quantity.Value/1852, LengthUnit.NauticalMile));Consistent would look like:
unitConverter.SetConversionFunction<Length>(LengthUnit.Meter, LengthUnit.Nanometer, quantity => new Length((quantity.Value) / 1e-9d, LengthUnit.Nanometer));
unitConverter.SetConversionFunction<Length>(LengthUnit.Meter, LengthUnit.NauticalMile, quantity => new Length(quantity.Value / 1852, LengthUnit.NauticalMile));There was a problem hiding this comment.
@tmilnthorp and I'm confused about backwards : I followed this guide and I saw this example :
Multiply for FromBaseToUnit and divide for FromUnitToBase, so that Length.Centimeter is defined as "FromBaseToUnit": "x*100" and "FromUnitToBase": "x/100" where base unit is Meter
It is an error in the guide or I was on the right way ?
There was a problem hiding this comment.
@angularsen I have been using spaces in the functions to match the styling of the rest of the code. Would you agree that the following is more consistent?
"FromUnitToBaseFunc": "{x} / 1e-10", "FromBaseToUnitFunc": "{x} * 1e-10",
Sure, we can do that. We should update existing files to make it consistent and update the wiki guide too then.
There was a problem hiding this comment.
@angularsen I have been using spaces in the functions to match the styling of the rest of the code. Would you agree that the following is more consistent?
"FromUnitToBaseFunc": "{x} / 1e-10", "FromBaseToUnitFunc": "{x} * 1e-10",Sure, we can do that. We should update existing files to make it consistent and update the wiki guide too then.
Will do
There was a problem hiding this comment.
@JasLieb I updated the wiki to maybe help clarify.
@tmilnthorp's point was that the functions are reversed. You would typically see this in the test
protected override double AngstromsInOneMeter => 1e-10;Does one meter equal 1e-10 angstroms?
There was a problem hiding this comment.
@JasLieb I updated the wiki to maybe help clarify.
@tmilnthorp's point was that the functions are reversed. You would typically see this in the test
protected override double AngstromsInOneMeter => 1e-10;Does one meter equal 1e-10 angstroms?
1 meters = 1e+10 Angstrom, So the overriden field should be :
protected override double AngstromsInOneMeter => 1e10;
If I change and if I apply @tmilnthorp review, I'm back on my feet, sound good to me and to the tests in this way.
|
@tmilnthorp Thank for your review ! But I ran into some issues if I follow all your comments : some tests are failing.
There is something am I doing wrong ? |
Uh oh. Looks like the BaseUnits is conflicting in the UnitSystem constructor since A happens to be first in the alphabetic list of quantity infos. This is a bug with our current UnitSystem code, not your PR. Remove the BaseUnits code for now. I'll make an issue of that. |
|
I added #1043 to track the issue with the UnitSystem code. |
… AngstromsInOneMeter value
|
@JasLieb Thanks for contributing, nuget should be out in a few minutes. |
closes #1039
Happy to do it on my own and glad to help with my first PR on this project !