No, it's just me, confusing the units. My apologies. False alarm.
It become apparent the moment I dig a bit into it.
No it's not atm vs bar confusion. It's confusion about what is 1 bar. The
root cause of confusion was a TDI book and my laziness. Before that I used
Pascals any time you have to get anything accurate, and thought of both atm
and bar as of somewhat historical artifacts from pre-SI days. I knew they
are roughly an atmospheric pressure but slightly different. Until one day
reading materials for my Tech class I stumble over a statement that 1 bar
IS DEFINED as 10 msw, which is the pressure of 10 meters of sea water. So I
though, aha, that's what it is. That's where that unit came from. Now, that
statement is true. 1 bar is 10 msw. The part they never mention and I never
bother to check is that msw is approximately a meter of sea water and I
thought it's literally the way how they defined that unit.
That was my mistake. Learned something new today.
Now, speaking of units, sure I found all definitions of depth_t,
pressure_t, e.t.c. well:
* Good idea in general, should be used everywhere.
* Parts of planner doesn't use those types and use int's instead. I even
found comments about getting rid of those, will do.
* Now, is the tough part. That all of those types has to be classes with
clearly defined operators
* All those converter functions has to be class members of units instead
of standalone functions. This is the only way to make those functions fool
proof.
That will simplify code by a lot. It should support mathematical operations
and comparison. If function return depth and you want to multiply it by
scalier to get depth you will end you with rather lengthy expression.
You can not just write *depth_t D = getDepth() * myScaler* and it gets
worse with cross unit operations. would be nice to just write
depth_t deltaD = getAscentRate() * getInterval();
currentDepth += deltaD;
In this case, first function will return rate_t, second - duration_t, and
then rate_t should have multiplying operator which return depth_t if
multiplied by unit of time. I mean, sure, you can get around with C syntax
but it's kinda long, harder to read and notice how int's start crawling
into the code.
int deltaD = getAscentRate().mms * getInterval().seconds;
currentDepth.mm += deltaD;
Another aspect is unit conversion. For instance. depth, should have all
readers to read the number as mm, inch, feet, mashing-machine-size you name
it. Cause currently nothing stops you to feed wrong unit into wrong helper
function. Also you can just do
pressure_t P = currentDepth.getPressure( dive );
This construction is fool proof. You cannot confuse units. And it's
readable in a glance. It's simple to do, but it will effectively nuke all
.c files, cause you'll loose ability to use those units from plain C. So I
guess it's not happening too soon.
Even right now, with the small changes I've got so far it has HUGE domino
effect on a code base. I'll try to minimize the number of changes, but
first submit will be kinda big.
As of your last question about ideas which lead to all this in a first
place. There's couple. Number one, and probably the biggest, is I would
like to see planner working on mobile. Which might lead to big rethinking
of UI. Editing profile the way we do right now in the planner will be a bit
to much for a small screen. Also, you don't have all the widgets you have
on PC. And some folks mention that they are afraid to use Subsurface
planner cause it looks too overwhelming. This is something I personally
disagree, but hey, there's that. If we can modify UI to make it a bit more
approachable. I mean, presets, hide some stuff under some "Show advanced
settings" checkbox, e.t.c. Anyway, all of that require planner to be more
modular. And right now it's more of a monolithic thing. One huge function
pulling all strings. Same goes about plannernotes.cpp, note generation is
again one-thing-that-does-it-all. Some people asked me whether it's
possible to add more explain-ability to the diveplan. That include many
things, but most of the time it will require ability to do calculations
while generating tables, while right now you only can use what planner have
prepared in advance.
On Wed, 19 Apr 2023 at 02:06, Robert.Helling <atdo...@posteo.net> wrote:
> 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.
>
>
--
Thanks.
Vlad A.
_______________________________________________
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
