On 8 April 2016 at 09:18, Rick Walsh <[email protected]> wrote: > Robert, > > On 7 April 2016 at 23:18, Robert Helling <[email protected]> wrote: > >> Rick, >> >> On 07.04.2016, at 10:37, Rick Walsh <[email protected]> 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. > > 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) {
> 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 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. > > Cheers, > > Rick >
_______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
