Loop icon indicating copy to clipboard operation
Loop copied to clipboard

Use full ICE history for displaying estimated carbohydrate effects

Open oliverstory opened this issue 1 year ago • 4 comments

The 'predicted' dataset on the 'glucose change' graph was calculated using insulin counteraction effects that had been truncated to the width of the chart. This meant that carb entries early in the timeline may not have dynamic carb absorption applied as the implied ICE was zero for the first part of the carb entry's history.

Addresses the different behaviour in landscape and portrait view of the glucose change graph in the carbohydrate screen. so that dynamic carb effects are always shown when they are in effect.

For discussion (but not included in this pull request), for clarity the graph dataset label could be changed from 'Predicted' to 'Estimated and predicted' given that it already incorporates dynamic glucose effects (when these are in effect), so is partly based on observation.

Does not address what I described as 'desired' behaviour of showing a static prediction for the 'predicted' dataset; I am not sure how you would achieve that.

Closes #2159

oliverstory avatar May 02 '24 12:05 oliverstory

I cannot reproduce the issue reported in #2159 with my test phone. I can apply the patch to dev, commit 47450c1. It applies cleanly, builds with no problem and the ICE screens are unchanged.

If there's an additional test that would be helpful, I could try again. (I ran for about 6 hours with 3 carb entries.) Do you know of specific configuration I could try to reproduce to make the inconsistency show up?

marionbarker avatar May 18 '24 20:05 marionbarker

Based on my understanding of the maths, to reproduce the issue prior to the patch you need:

  1. A carb entry where ICE is slightly larger than minimum rate absorption but not hugely larger.
  2. To set this up on a test phone you could try adding an insulin entry and have glucose flat. Add a matching carb entry at a similar time. Make the carb entry slightly smaller than you'd normally have for that amount of insulin (or conversely, make the insulin entry slightly larger than your settings would recommend for that carb amount). You could enter this at a time close to the earliest time on the portrait view glucose change graph to make subsequent steps shorter.
  3. Carb entry should initially show dynamic carb absorption - ie ‘predicted’ curve will follow ‘observed’ curve - on both graphs.
  4. Wait until the start time of the carb entry moves off the left of the portrait view graph. At a certain point, the sum of minimum rate absorption over time for the entry will be more than the sum of ICE (because prior to the patch, ICE is truncated at the left edge of the graph). Now the predicted curve will follow minimum rate absorption and be below ICE on the portrait view. But on landscape view, the it should still show dynamic carb absorption (because the landscape view uses a longer series of ICE values).

oliverstory avatar May 18 '24 21:05 oliverstory

I would like to try this, if I use your commt number and git hub info and add line to my yum file, would that work for browser build?

Trpl7ca avatar Jul 11 '24 17:07 Trpl7ca

Yes it would work (for dev branch, not for main branch). You can see the lines you need in my browser build yum file.

On Fri, 12 Jul 2024 at 3:57 AM, Trpl7ca @.***> wrote:

I would like to try this, if I use your commt number and git hub info and add line to my yum file, would that work for browser build?

— Reply to this email directly, view it on GitHub https://github.com/LoopKit/Loop/pull/2163#issuecomment-2223545502, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFRGMJ5Y35V2ZCDEY7J7OTZL3BPDAVCNFSM6AAAAABHDS4YQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRTGU2DKNJQGI . You are receiving this because you authored the thread.Message ID: @.***>

oliverstory avatar Jul 11 '24 22:07 oliverstory

This code will change with the tidepool-merge (and I think this display bug may not exist in that branch, from reading the code - I have not tested tidepool-merge)

oliverstory avatar Mar 31 '25 10:03 oliverstory

I did test tidepool-merge (if you look at LoopWorkspace PR 513, you will see some testing summary comments.)

We plan a new release from dev before merging tidepool-merge. Whether this PR is merged as part of that process is under discussion.

marionbarker avatar Mar 31 '25 13:03 marionbarker