Round gas depth properly #3357
Round gas depth properly #3357
Conversation
|
👍 Id love to see the units in struct plot_data also be changed to depth_t , duration_t and so on, but that might be another PR. |
core/profile.c
Outdated
| fn2 = (int)(1000.0 * entry->pressures.n2 / amb_pressure); | ||
| fhe = (int)(1000.0 * entry->pressures.he / amb_pressure); | ||
| fn2 = (1000.0 * entry->pressures.n2 / amb_pressure); | ||
| fhe = (1000.0 * entry->pressures.he / amb_pressure); |
There was a problem hiding this comment.
Small style comment: You could remove the parentheses in the previous two lines.
No comment on the rest - this is above my pay-grade. Turning these two into floating points looks reasonable though.
There was a problem hiding this comment.
you still could drop those parenthesis, right?
atdotde
force-pushed
the
doublefraction
branch
3 times, most recently
from
8f942de
to
bccce0f
Compare
core/profile.c
Outdated
| entry->ead = (entry->depth + 10000) * fn2 / (double)N2_IN_AIR - 10000; | ||
| entry->eadd = (entry->depth + 10000) * | ||
| entry->mod = gas_mod(gasmix, modpO2, dive, 1).mm; | ||
| entry->end = mbar_to_depth((int)(depth_to_mbarf(entry->depth, dive) * (1000 - fhe) / 1000.0), dive); |
There was a problem hiding this comment.
This code is mathematically wrong.
(int)(...) will round 100.0-100.0*eps (99.9999999...) to 99.
You want lrint(...) or (int)(round(...)), as I have stated before.
Of course, that will still give the same rounding problem just for 100.5 +/- 100.5 * eps. But at least the result is not outright wrong.
To fully avoid the problem, I reckon mbar_to_depth should also exist as version taking a double.
There was a problem hiding this comment.
You are right, the (int) cast always rounds down not to nearest value.
I don't think the problem at 100.5 is as big as we have many situations where we are at an int (e.g. END for nitrox dives) then do calculation that for real numbers is a NOOP but not for floating point and for (int) we were doing that at the edge of the discontinuity. But I would expect much fewer cases where we are sitting close the the .5 edge.
I disagree, though that a double version of mbar_to_depth would help: Then, in the test, we would be comparing doubles for equality which is bad, comparing the rounded ints has much better chance of not picking up noise.
There was a problem hiding this comment.
I disagree, though that a double version of mbar_to_depth would help: Then, in the test, we would be comparing doubles for equality which is bad, comparing the rounded ints has much better chance of not picking up noise.
You misunderstood. I'm not suggesting to return a double, but to take a double argument. The fundamental problem in all this code is intermittent conversion to integral types. Fixed-point and floating-point are both fine, if done correctly. It's the combination that ends up in a disaster.
atdotde
force-pushed
the
doublefraction
branch
from
bccce0f
to
aae7733
Compare
| @@ -41,7 +41,7 @@ const char *divemode_text_ui[] = { | |||
| // For writing/reading files. | |||
| const char *divemode_text[] = {"OC", "CCR", "PSCR", "Freedive"}; | |||
|
|
|||
| static int calculate_depth_to_mbar(int depth, pressure_t surface_pressure, int salinity); | |||
| static double calculate_depth_to_mbarf(int depth, pressure_t surface_pressure, int salinity); | |||
There was a problem hiding this comment.
any function name ending on barf makes me happy
core/profile.c
Outdated
| fn2 = (int)(1000.0 * entry->pressures.n2 / amb_pressure); | ||
| fhe = (int)(1000.0 * entry->pressures.he / amb_pressure); | ||
| fn2 = (1000.0 * entry->pressures.n2 / amb_pressure); | ||
| fhe = (1000.0 * entry->pressures.he / amb_pressure); |
There was a problem hiding this comment.
you still could drop those parenthesis, right?
The D in MOD, EAD, END, and EADD stands for "depth" and as such these should be mm in int rather than double. The intermediate fn2 and fhe2, however, as intermediate value should not be rounded to an integer. The upshot of this is a litle more numerical stability. It should lead to more stable values in TestProfile when run on architectures with different floating point precision. Signed-off-by: Robert C. Helling <helling@atdotde.de>
atdotde
force-pushed
the
doublefraction
branch
from
aae7733
to
f79ce0d
Compare
|
Argh, I had removed the parentheses in the second commit that worked around the trivial cases and that I thew away after @bstoeger's comment. Re "barf": I first encountered that term a long time ago in "The unix haters handbook" (https://en.wikipedia.org/wiki/The_UNIX-HATERS_Handbook) that came with a "barf bag" which was still there in pristine state in our library's copy. |
|
This seems to resolve the rounding problems encountered in TestProfile on different architectures. So let's merge. |
|
Just an idle comment, feel free to ignore:
I think this is the wrong way to think about it. The tests are there to find questionable code, it's not the code that is there to pass the tests. It's the same thing with compiler warnings (and other validation techniques) that many people get wrong. In this case the tests alerted us to questionable code and I still think that the current version is wrong: It truncates mid-calculation to full mbar, which has less precision than the resulting mm. However, since the imprecision is below the precision of the measured data I don't really care. |
The D in MOD, EAD, END, and EADD stands for "depth" and
as such these should be mm in int rather than double.
The intermediate fn2 and fhe2, however, as intermediate
value should not be rounded to an integer.
The upshot of this is a litle more numerical stability.
It should lead to more stable values in TestProfile
when run on architectures with different floating
point precision.
Signed-off-by: Robert C. Helling helling@atdotde.de
Describe the pull request:
Pull request long description:
Changes made:
Related issues:
Additional information:
Release note:
Documentation change:
Mentions: