[PATCH] Profile VPM-B ceiling: ignore negative deco time

[Rick Walsh]

On 8 April 2016 at 09:37, Rick Walsh <rickmwalsh at gmail.com> wrote:

>
>
> On 8 April 2016 at 09:18, Rick Walsh <rickmwalsh at gmail.com> wrote:
>
>> Robert,
>>
>> On 7 April 2016 at 23:18, Robert Helling <helling at atdotde.de> wrote:
>>
>>> Rick,
>>>
>>> On 07.04.2016, at 10:37, Rick Walsh <rickmwalsh at gmail.com> wrote:
>>>
>>> The variable deco_time is used for the CVA iterations to calculate the
>>> VPM-B
>>> ceiling. Only positive values should be allowed, otherwise we can get
>>> some odd
>>> results in a few cases, such as ceilings being calculated when they
>>> shouldn't.
>>>
>>>
>>> I think your analysis was only partially correct. Indeed the problem
>>> occurred when the deco time got negative.
>>>
>>> But looking at the code, this should not happen (it would be negative if
>>> the deepest ceiling occurred after the ceiling had cleared). So your patch
>>> does not address the actual problem, only the symptom.
>>>
>>> As I understand it, the rounding of ceilings to multiples of 3m is
>>> really to blame: In the if clause to find the deepest ceiling, there was a
>>> >= in the ceiling comparison. So “deepest” was also found when the ceiling
>>> was constant at 0m, which made time_deep_ceiling the end of the dive.
>>>
>>> Changing that >= to > does not solve the problem either since that is
>>> far too early (in the xml provided, the first time there was any non-zero
>>> ceiling as that was rounded to 3m). The way out is really to take the
>>> ceiling without rounding to multiples of 3m. This si what this patch does
>>> (unfortunately, it also fixes whitespace damage).
>>>
>>>
>>>
>>> Could you please look at this patch and confirm or dispute my solution?
>>>
>>>
>>> I have looked at the patch, but I've not had a chance to compile and run
>> it.  Can I assume you tested John's xml, and it fixed the problem?  From
>> what I can see, there should be a small ceiling when the conservatism is
>> +1, and no ceiling when it's +0.  Don't get confused (I did at first) by
>> the red ceiling, i.e. DC reported, which now always prints - it should
>> always print on mobile, but on desktop whether or not it prints should be
>> determined whether that option has been selected.
>>
>> I agree that we should not consider a ceiling of 0 m.  Using the actual
>> calculated ceiling, rather than the ceiling rounded to 3 m, is more
>> rational.  I can't recall, but we might have decided to use (in the planner
>> VPM-B) the rounded ceiling because it was more consistent with other
>> implementations.  I could be wrong on that point but it was something that
>> I definitely considered last year.  If not rounding the ceiling still shows
>> essentially the same ceiling for planned dives after they have been saved,
>> I say we should definitely use the not-rounded ceiling.  Either way, we
>> should use the same value whether or not the user selects the "show
>> ceilings rounded the 3m" option.
>>
>
I've applied and tested your patch.  I believe it is correct, and as you
pointed out, mine just masked the real issue.  I've re-attached yours
(patch 1/2) with the directory subsurface-core renamed to core, so that it
applies to master.  Otherwise it's unchanged.

>
>> Using >= was deliberate but perhaps misguided. I wanted to pick up the
>> last time the ceiling was deepest.  Imagine a dive which starts quite deep
>> (deepest ceiling occurs at some point), the diver ascends (ceiling becomes
>> shallower), diver stays at the same depth or goes deeper until the previous
>> deepest ceiling is reached but not exceeded, before starting the final
>> ascent.  I wanted to consider the second time the deepest ceiling is
>> reached as the end of the bottom phase of the dive, and base the CVA from
>> that.  However, if we don't round the ceiling, such that it is calculated
>> to the mm, the likelihood of reaching exactly the same ceiling is so
>> unlikely, and the consequence is minor, that using > rather than >= is
>> worthwhile to avoid the problem with rounded ceilings and zero ceiling.
>>
> An alternative conditional that would work both ways could be:
>      if  ((entry->ceiling > 0) && (entry->ceiling >= first_ceiling) {
>
> After considering and testing this condition, I decided it was harmless
but of no benefit.

>
>> I only had a few minutes to look at what as happening with John's dives.
>> My initial impression was (for the problem dive with +0 conservatism):
>> - on the initial iteration runs, with the initial gradients (most
>> conservative - ignoring the CVA benefit), a shallow ceiling was calculated
>> - on the CVA iteration, no ceiling was calculated because the allowable
>> gradients are a little more aggressive, but time_clear_ceiling was the
>> value from previous iteration, which meant that deco_time was negative (I
>> could be completely wrong - it depends on when time_clear_ceiling is
>> defined / initialized)
>> - on the next CVA iteration, the updated gradients were wrong because of
>> the negative deco_time
>>
>> I'm still a bit concerned about the variables controlling the VPM-B and
CVA witchcraft being declared outside the CVA loop and not re-initialized.
I wasn't able to find or create a case where this caused a problem, but I
think it's safer to declare the variables at the start of the loop.  The
attached patch (2/2) does this.


> I must confess, my debugging was very limited.  I really only got as far
>> as realizing that deco_time was negative, so tried to make that
>> impossible.  My simple change fixed the problem with John's dives, so it
>> must be right (jk).
>>
>
If your patch works, and doesn't make the calculated ceiling look incorrect
>> for planned dives, and other real dives (especially paying attention to
>> dives with only very shallow ceilings), I think you have the right
>> solution.  We could apply both patches, but would want to be reasonably
>> confident that my patch isn't just masking a problem rather than fixing it.
>>
>>
I'm now convinced that my original patch did just mask a bug.  It should
not be applied.


> Cheers,
>>
>> Rick
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20160409/38de9fd5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-VPM-B-profile-declare-CVA-iteration-variables-within.patch
Type: text/x-patch
Size: 1983 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20160409/38de9fd5/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-time-of-first-ceiling-calculation.patch
Type: text/x-patch
Size: 3632 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20160409/38de9fd5/attachment-0003.bin>