On 8 April 2016 at 09:37, Rick Walsh <[email protected]> wrote:
>
>
> 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.
>>
>
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
>>
>
>
From 015310f51618640cf72eb1e24374c9edff9ba7fd Mon Sep 17 00:00:00 2001
From: Rick Walsh <[email protected]>
Date: Sat, 9 Apr 2016 09:22:46 +1000
Subject: [PATCH 2/2] VPM-B profile: declare CVA iteration variables within
each loop
The variables that control each CVA iteration should be declared at the start
of each loop so that the values are carried over from one iteration to the
next.
Signed-off-by: Rick Walsh <[email protected]>
---
core/profile.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/core/profile.c b/core/profile.c
index c0042ed..b4f7279 100644
--- a/core/profile.c
+++ b/core/profile.c
@@ -925,10 +925,8 @@ void calculate_deco_information(struct dive *dive, struct divecomputer *dc, stru
{
int i, count_iteration = 0;
double surface_pressure = (dc->surface_pressure.mbar ? dc->surface_pressure.mbar : get_surface_pressure_in_mbar(dive, true)) / 1000.0;
- int last_ndl_tts_calc_time = 0;
- int first_ceiling = 0, current_ceiling;
bool first_iteration = true;
- int final_tts = 0 , time_clear_ceiling = 0, time_deep_ceiling = 0, deco_time = 0, prev_deco_time = 10000000;
+ int deco_time = 0, prev_deco_time = 10000000;
char *cache_data_initial = NULL;
/* For VPM-B outside the planner, cache the initial deco state for CVA iterations */
if (prefs.deco_mode == VPMB && !in_planner())
@@ -936,6 +934,7 @@ void calculate_deco_information(struct dive *dive, struct divecomputer *dc, stru
/* For VPM-B outside the planner, iterate until deco time converges (usually one or two iterations after the initial)
* Set maximum number of iterations to 10 just in case */
while ((abs(prev_deco_time - deco_time) >= 30) && (count_iteration < 10)) {
+ int last_ndl_tts_calc_time = 0, first_ceiling = 0, current_ceiling, final_tts = 0 , time_clear_ceiling = 0, time_deep_ceiling = 0;
for (i = 1; i < pi->nr; i++) {
struct plot_data *entry = pi->entry + i;
int j, t0 = (entry - 1)->sec, t1 = entry->sec;
--
2.5.5
From e417f7c31007184e5bba4f7c24e87ff3fc08e646 Mon Sep 17 00:00:00 2001
From: "Robert C. Helling" <[email protected]>
Date: Thu, 7 Apr 2016 15:06:44 +0200
Subject: [PATCH 1/2] Fix time of first ceiling calculation
In our verision of VPM-B for real dives, we take as the deco time the
difference between the time of the deepest ceiling and the time when the
ceiling clears.
When the display of ceilings was set to multiples of 3m this was confused, as
the maximum finder had issues: First of all, it updated the time when the ceiling
was the same (which was almost always the case for stepped ceilings) but changing
>= to > was not enough, since then the first time a deepest stepped ceiling was
reached was used.
This patch uses the actual ceiling (not rounded to the next integer multiple of 3m)
for this calculation to get rid of this problem.
Signed-off-by: Robert C. Helling <[email protected]>
---
core/profile.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/core/profile.c b/core/profile.c
index 72c5e02..c0042ed 100644
--- a/core/profile.c
+++ b/core/profile.c
@@ -926,7 +926,7 @@ void calculate_deco_information(struct dive *dive, struct divecomputer *dc, stru
int i, count_iteration = 0;
double surface_pressure = (dc->surface_pressure.mbar ? dc->surface_pressure.mbar : get_surface_pressure_in_mbar(dive, true)) / 1000.0;
int last_ndl_tts_calc_time = 0;
- int first_ceiling = 0;
+ int first_ceiling = 0, current_ceiling;
bool first_iteration = true;
int final_tts = 0 , time_clear_ceiling = 0, time_deep_ceiling = 0, deco_time = 0, prev_deco_time = 10000000;
char *cache_data_initial = NULL;
@@ -973,22 +973,26 @@ void calculate_deco_information(struct dive *dive, struct divecomputer *dc, stru
vpmb_next_gradient(deco_time, surface_pressure / 1000.0);
}
entry->ceiling = deco_allowed_depth(tissue_tolerance_calc(dive, depth_to_bar(entry->depth, dive)), surface_pressure, dive, !prefs.calcceiling3m);
+ if (prefs.calcceiling3m)
+ current_ceiling = deco_allowed_depth(tissue_tolerance_calc(dive, depth_to_bar(entry->depth, dive)), surface_pressure, dive, true);
+ else
+ current_ceiling = entry->ceiling;
/* If using VPM-B outside the planner, take first_ceiling_pressure as the deepest ceiling */
if (prefs.deco_mode == VPMB && !in_planner()) {
- if (entry->ceiling >= first_ceiling) {
- time_deep_ceiling = t1;
- first_ceiling = entry->ceiling;
- first_ceiling_pressure.mbar = depth_to_mbar(first_ceiling, dive);
- if (first_iteration) {
- nuclear_regeneration(t1);
- vpmb_start_gradient();
- /* For CVA calculations, start by guessing deco time = dive time remaining */
- deco_time = pi->maxtime - t1;
- vpmb_next_gradient(deco_time, surface_pressure / 1000.0);
- }
+ if (current_ceiling > first_ceiling) {
+ time_deep_ceiling = t1;
+ first_ceiling = current_ceiling;
+ first_ceiling_pressure.mbar = depth_to_mbar(first_ceiling, dive);
+ if (first_iteration) {
+ nuclear_regeneration(t1);
+ vpmb_start_gradient();
+ /* For CVA calculations, start by guessing deco time = dive time remaining */
+ deco_time = pi->maxtime - t1;
+ vpmb_next_gradient(deco_time, surface_pressure / 1000.0);
+ }
}
// Use the point where the ceiling clears as the end of deco phase for CVA calculations
- if (entry->ceiling > 0)
+ if (current_ceiling > 0)
time_clear_ceiling = 0;
else if (time_clear_ceiling == 0)
time_clear_ceiling = t1;
--
2.5.5
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
