-
Notifications
You must be signed in to change notification settings - Fork 392
Relative and absolute equality #451
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
Changes from all commits
ce4c327
4272747
877db67
35a8d4c
74c3faa
9fe2dbc
2ac0600
b2898c7
882a4d7
490e5af
2a50b72
241cabc
06164a3
94be718
5c5db3c
62e3424
2071848
760f17f
fb04723
d35958c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,11 +42,11 @@ public class MassTests : MassTestsBase | |
|
||
protected override double KilotonnesInOneKilogram => 1E-6; | ||
|
||
protected override double LongTonsInOneKilogram => 0.000984207; | ||
protected override double LongTonsInOneKilogram => 9.842065276110606e-4; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR is getting a bit crowded and not so easy to review. Could you perhaps pull out the bits not directly related to absolute/relative functionality in a separate PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately all of these changes are so the tests pass. These validation numbers were off quite a bit when I changed the tests to validate by % tolerance (not precise enough). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in other words, they're unfortunately all related to absolute/relative functionality! Especially since I have switched the tests over to relative tolerance by default. Without the mass tests change, mass tests fail with the following (MegatonnesInOneKilogram in this example):
As you can see, it was a difference of 1e-6, which was inside the 1e-5 tolerance, but it was off by almost double!! 😲 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha 😆 it's so good to finally fix this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this getting ready now? I might find some time tomorrow to review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup! I'm done making changes. |
||
|
||
protected override double MegapoundsInOneKilogram => 2.2046226218487757e-6; | ||
|
||
protected override double MegatonnesInOneKilogram => 1E-6; | ||
protected override double MegatonnesInOneKilogram => 1E-9; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow.. Just wow! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know! |
||
|
||
protected override double MicrogramsInOneKilogram => 1E9; | ||
|
||
|
@@ -60,7 +60,7 @@ public class MassTests : MassTestsBase | |
|
||
protected override double PoundsInOneKilogram => 2.2046226218487757d; | ||
|
||
protected override double ShortTonsInOneKilogram => 0.00110231; | ||
protected override double ShortTonsInOneKilogram => 1.102311310924388e-3; | ||
|
||
protected override double StoneInOneKilogram => 0.1574731728702698; | ||
|
||
|
@@ -105,4 +105,4 @@ public void MassTimesAccelerationEqualsForce() | |
Assert.Equal(force, Force.FromNewtons(54)); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value was different than Years365 and the average year instead of 365 days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying it is supposed to be different? Both are defines in the json files as x36524*3600, so I am not sure how they could be?