regressions because of the new tank handling

[Linus Torvalds]

On Thu, Sep 14, 2017 at 1:36 AM, Stefan Fuchs <sfuchs at gmx.de> wrote:
>
> First of all I have to admit that maybe this "spike in the pressure graph"
> item doesn't really fit together with the other things I reported.  I don't
> believe that this item is also linked to the change Linus did. I even tend
> to say that someone already months ago saw such spikes in the graph,
> reported them but then the issue somehow disappeared again.

Hmm. It might be new, it might be old, it's an interesting dive plan,
and I could see why it's doing that (and I could see how the old code
might do the same thing, but also how the new code might just make it
more obvious, so it really could go either way).

What's going on is two-fold:

 (a) it's not a "real" dive with real samples, but a planned one, and
that means that the "samples" are really planning way-points, and are
fairly far away from each other.

 (b) the gas change events and the pressures saved are ambiguous.

The first issue (a) is not really the cause of anything, but it is
what makes things more noticeable: because the "samples" are far away
from each other, the ambiguity that comes from (b) is much more
noticeable.

The real issue is (b), which is definitely not new, but it's possible
that the old code happened to show that ambiguous case differently.

To explain (b), lI'll just show the samples:

  <sample time='32:00 min' depth='24.0 m' pressure='127.003 bar' />
  <sample time='33:00 min' depth='21.0 m' pressure='124.973 bar' />
  <sample time='33:01 min' depth='21.0 m' pressure='122.341 bar' />
  <sample time='38:00 min' depth='21.0 m' pressure='145.888 bar' />
  <sample time='42:00 min' depth='9.0 m' pressure='132.817 bar' />
  <sample time='44:00 min' depth='9.0 m' pressure='127.889 bar' />

Note how the "pressures" up until 33:01 are obviously with one tank,
and then the pressures from 38:00-44:00 are with another.

So which tank is it? We don't actually say, because out old XML format
never needed to: we only supported one pressure sensor, and we'd use
the cylinder change events to say which tank it was.

But then look at the events we have (edited to remove the type/value
information that is internal random numbers):

  <event time='10:01 min' name='gaschange' cylinder='1' o2='18.0%' he='45.0%' />
  <event time='33:01 min' name='gaschange' cylinder='0' o2='50.0%' he='15.0%' />
  <event time='45:01 min' name='gaschange' cylinder='2' o2='100.0%' />

note how that 33:01 mark is when we change gas. So the embiguity is
"which tank pressure do we have *at* 33:01?"

The odd spike comes from the fact that subsurface now takes that tank
pressure reading at 33:01 to be the *new* tank, not the old talk. But
the new tank then goes up to 145 bar in the next sample at 38:00, so
you get that really odd transition from 122 back up to 145.

So I do think it's a bug, but it's a bug not in how we show things,
but in how the planner has generated those events and pressures. The
planner apparently generated that ambiguous thing.

Note that the same thing does *not* happen at the other gas change
events, because those have no pressure associated with them at all:

  <sample time='10:01 min' depth='15.0 m' />
  <sample time='45:01 min' depth='6.0 m' />

and I don't know why that 33:01 sample does (well, I do _kind_ of
know: we're switching back to a gas we already used, and we probably
have some code that just ends up messing that up).

Again, I think it's a planner thing, although it could easily be an
artifact of a planner interaction with the cylinder pressure
calculations for the profile that may have come in with the new code
(ie the old code might have _hidden_ the ambiguity almost by mistake).

And just removing the pressure reading from that event at 33:01 makes
the graph show correctly without any odd spikes, but that's obviously
a workaround for the bug, not a fix.

However, while playing around with an alternate fix (just stating the
sensor explicitly in the xml file), I note that we seem to get that
case wrong too, and that might in fact be a bug in the new code. I
will investigate (although it might have been just me also messing up
the xml file editing by hand ;)

                    Linus