Vlad,
> On 19. Apr 2023, at 02:51, Vlad A <elf...@gmail.com> wrote:
>
> Ok, here's question.
>
> Should I fix bugs on the way? Cause I start finding some. It's errors in core
> math in dive.c
> For instance someone have confused units and assumes 1 bar = 100kPa, so if
> you call calculate_depth_to_mbarf( 10000, { 1000 }, 10300 ), which by
> definition should return 2000.0 mbar. It will actually give you 2010.43 which
> is wrong.
> Fixing those will likely cause tests to fail and brake the requirement
> "Whatever we change we need to make sure we are not breaking things too much
> not only in the most obvious ways to use the planner but also in the many
> niche ways that people now rely on it." since that bug might be a feature
> already.
>
> So far, I'm only fixing those that will not change numbers and just leaving
> notes everywhere I can see potential issues, but at some point we have to
> talk about those.
yes, please fix bugs. But you need to be sure that these are bugs. In this
case, it is not (which you could see if you looked at the code for the
function): Subsurface takes great care to do things „correctly“. In this case,
it takes the actual surface pressure and the actual density of water into
account when converting depths to pressure (it computes the weight of the water
column above the diver), 1 m of depth thus does not equal 1 bar of additional
pressure, only approximately. Similarly, when computing gas amounts, 1 litre of
gas in your cylinder with x bar does not equal x litres of gas expanded to 1
bar, rather it takes a real gas law (and thus the composition of the gas) into
account.
This is why there are all these helper functions in dive.c that do these more
complicated conversions for you so you don’t have to worry about these things
in the more higher level logic of the planner for example. Just remember to use
these helper functions and not do conversions directly.
For this, it helps to take units seriously (here pressure vs. depth): Whenever
you convert between units, use a helper rather than simply multiplying by a
conversion factor.
Speaking of units: There is a general rule (with exceptions, see below) in
Subsurface to internally use „standard units“ which allow you to fit all
typical values in an int. This means, all times are seconds, all lengths
(depths) are millimetres, all pressures are millibars. We even have types for
those like
typedef struct
{
int32_t seconds; // durations up to 34 yrs
} duration_t;
in units.h, so you have to write my_duration.seconds to access the value to
remind you of this.
There are exceptions to this rule, in particular in the deco parts deco.c/h.
This is partly for historical reasons (since we started from absorbing code
from another open source project), but also not to introduce numerical errors
(we track tiny changes in tissue pressures that can accumulate over time), so
ints are not appropriate but we have to use floating point numbers. But you
should do this only consciously when you know an int will not do.
And while we are at it: Please structure your changes into meaningful, small
commits: In the end, not only the compiler but humans (in that case that in
particular means: me) will have to understand what you are doing and be
convinced this is the correct thing. Of course, after each commit the program
still needs to build without errors (as otherwise bisecting will become very
painful).
Finally, please have these discussion on the mailing list and not just amongst
a few people.
Best
Robert
PS: I am still interested in what you are planning to implement in the end.
Maybe discussing this first might be a good idea before you invest a lot of
time.
_______________________________________________
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
