fix(lib/iob): Move value checks up to index.js #1437

ChanceHarrison · 2022-10-23T01:57:42Z

(Mostly) fixes #1436

The root cause of the above issue is that the logic for using and validating custom insulinPeakTime (and by extension, the errors that are logged from said logic) was all the way down (in the sense of a call stack) in calculate.js. This means that the validation logic was running many more times than necessary (and in the case of when the values fall out of the tolerable range, an absurd amount of log messages).

Here's what the typical flow might look like for that:

oref0-pump-loop.sh defines and invokes calculate_iob
calculate_iob calls run_remote_command 'oref0-calculate-iob [...]
oref0-calculate-iob.js uses ../lib/iob as generate
lib/iob/index.js uses lib/iob/total.js(called either 1 or 48 times depending on bool currentIOBOnly)
lib/iob/total.js uses lib/iob/calculate.js (called for each non-future treatment that is more recent than dia_ago)

The changes I made move such value handling (both that in calculate.js and in total.js; thought the former was the source of pain, both were moved for consistency) to index.js, where the handling can be done once and the values can be used many times in calcualte.js and total.js.

As this change asserts, why would we fetch the values, validate them, and log on error in calculate.js (which can, in the worst case be called 48 * the number of treatments that are being evaluated in the DIA times) when we can do so once in lib/iob/index.js and just pass the values down?

This is a much better situation, because we only get one log message for each invocation of lib/iob.

The fix isn't perfect (and why I say it only mostly fixes #1436), because invoking oref0-meal results in somewhere between 15 and 20 lib/iob invocations, and thus an equivalent number of log messages. Not ideal, but much more palatable than the original number.

oref0-pump-loop.sh defines and invokes refresh_pumphistory_and_meal
refresh_pumphistory_and_meal calls run_remote_command 'oref0-meal [...]
oref0-meal.js users ../lib/meal as generate
lib/meal/index.js uses lib/meal/total.js (called once)
lib/meal/total.js uses lib/determine-basal/cob.js as detectCarbAbsorption (called for each treatment + 1)
lib/determine-basal/cob.js uses lib/iob as get_iob (once for each bucketed_data value, seems to be between 15 to 20 times on average)

Arguably a similar (if not more effective) fix for the logging issue would be to have just commented out the lines that were doing the logging entirely, but getting rid of one of the only ways that the user might know their config has out-of-range values didn't feel good.

On that note, I think it might be worth considering how we can get the log message into the pump-loop log instead of just the shared-node log, since I don't think there is much (if any) documentation that directs users to look in the shared-node log.

That, and maybe the pump-loop should just fail on out-of-range config values? It would certainly be more noticeable, but maybe also not desirable since it's an issue that we can correct programmatically by using the min/max allowed value.

0.7.1

scottleibrand · 2022-10-23T02:04:54Z

@mountrcg can you test this and confirm whether it helps with your issue, and otherwise works properly?

ChanceHarrison · 2022-10-23T02:05:54Z

I should probably mention that I'm running with these changes on my rig. I haven't noticed any issues, but I'm happy to run it for a few days and confirm that is still the case.

mountrcg · 2022-12-08T08:03:52Z

just saw this now and will test, great stuff @ChanceHarrison

mountrcg · 2022-12-08T10:03:36Z

warnings have been reduced to:

2022-12-08 10:32:53.279824+0100 FreeAPS[36957:2018777] [OpenAPS] JavaScriptWorker.swift - createContext() - 23 DEV: JavaScript log: Warning: adjusting insulin peak time from,48,to a minimum of 50m for,rapid-acting,insulin
2022-12-08 10:32:53.281234+0100 FreeAPS[36957:2018777] [OpenAPS] JavaScriptWorker.swift - createContext() - 23 DEV: JavaScript log: Warning: adjusting insulin peak time from,48,to a minimum of 50m for,rapid-acting,insulin
2022-12-08 10:32:53.282051+0100 FreeAPS[36957:2018777] [OpenAPS] JavaScriptWorker.swift - createContext() - 23 DEV: JavaScript log: Warning: adjusting insulin peak time from,48,to a minimum of 50m for,rapid-acting,insulin
2022-12-08 10:32:53.282804+0100 FreeAPS[36957:2018777] [OpenAPS] JavaScriptWorker.swift - createContext() - 23 DEV: JavaScript log: Warning: adjusting insulin peak time from,48,to a minimum of 50m for,rapid-acting,insulin
2022-12-08 10:32:53.283486+0100 FreeAPS[36957:2018777] [OpenAPS] JavaScriptWorker.swift - createContext() - 23 DEV: JavaScript log: Warning: adjusting insulin peak time from,48,to a minimum of 50m for,rapid-acting,insulin
2022-12-08 10:32:53.284770+0100 FreeAPS[36957:2018777] [OpenAPS] JavaScriptWorker.swift - createContext() - 23 DEV: JavaScript log: Warning: adjusting insulin peak time from,48,to a minimum of 50m for,rapid-acting,insulin
2022-12-08 10:32:53.454443+0100 FreeAPS[36957:2018777] [OpenAPS] JavaScriptWorker.swift - createContext() - 23 DEV: JavaScript log: Warning: adjusting insulin peak time from,48,to a minimum of 50m for,rapid-acting,insulin

but thats on the simulator, I might not have enough treatments on it. will test on live data shortly.

mountrcg · 2022-12-08T10:07:15Z

same result on live system, 7 warnings only

mountrcg · 2022-12-08T10:15:29Z

lovely, fixed in my book!

When Insulin Peak Time is set too low o high.

scottleibrand · 2022-12-08T17:32:16Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "oref0",
-  "version": "0.7.0",
+  "version": "0.7.1",


I think this is an artifact? Will need to check it before merging.

I'm not sure! All I can say is that it wasn't in my original changeset.

It was because you based your change on master instead of dev, so shows up as a diff after I rebased to dev.

Ah, of course. My bad on the base branch. Fortunately, it doesn't seem to make a significant difference in this context (beyond the version diff in package.json). Let me know if you would like me to do anything on my end.

I'll probably git reset --hard origin/dev and cherry-pick your commits, then git push -f it, unless you beat me to it.

scottleibrand · 2022-12-09T03:56:17Z

Merged via abf9c6b

scottleibrand · 2023-03-05T03:29:17Z

I had to revert this: 10f9e27

It is causing future IOB predictions to be incorrect (highly negative).
That causes the predBG lines to swoop up and to the right.
That results in (slightly) too much insulin being dosed in some situations.

scottleibrand · 2023-03-06T02:22:57Z

To repro, run oref0-calculate-iob and look at the resulting iob.json. Note the iob values at the very end of the json file. With current dev (and the version previous to the regression), they'll be near zero. With the regression, they'll be highly negative.
oref0-calculate-iob monitor/pumphistory-24h-zoned.json settings/profile.json monitor/clock-zoned.json settings/autosens.json | jq . > iob.verbose.json

-    "iob": -1.52,
-    "activity": -0.0196,
-    "basaliob": -1.53,
+    "iob": 0.012,
+    "activity": 0.0006,
+    "basaliob": 0.002,
     "bolusiob": 0.01,
-    "netbasalinsulin": -4.45,
+    "netbasalinsulin": 0.2,

danamlewis and others added 3 commits June 18, 2022 19:13

Merge pull request #1317 from openaps/dev

69d7673

0.7.1

0.7.1

88cf032

fix(lib/iob): Move value checks up to index.js

ff74e24

Jon-b-m added a commit to Jon-b-m/freeaps that referenced this pull request Dec 8, 2022

Fix from openaps/oref0#1437

17afdac

When Insulin Peak Time is set too low o high.

scottleibrand approved these changes Dec 8, 2022

View reviewed changes

scottleibrand changed the base branch from master to dev December 8, 2022 17:31

scottleibrand reviewed Dec 8, 2022

View reviewed changes

scottleibrand added a commit that referenced this pull request Dec 9, 2022

Merge branch 'ChanceHarrison-chance/fix-1436' (#1437) into dev

abf9c6b

scottleibrand closed this Dec 9, 2022

ChanceHarrison deleted the chance/fix-1436 branch December 9, 2022 04:27

scottleibrand mentioned this pull request Mar 5, 2023

Excessive limitation checks and logging when Custom Peak time below limit of active insulin curve #1436

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lib/iob): Move value checks up to index.js #1437

fix(lib/iob): Move value checks up to index.js #1437

ChanceHarrison commented Oct 23, 2022 •

edited

Loading

scottleibrand commented Oct 23, 2022

ChanceHarrison commented Oct 23, 2022

mountrcg commented Dec 8, 2022

mountrcg commented Dec 8, 2022

mountrcg commented Dec 8, 2022

mountrcg commented Dec 8, 2022

scottleibrand Dec 8, 2022

ChanceHarrison Dec 8, 2022

scottleibrand Dec 8, 2022

ChanceHarrison Dec 8, 2022 •

edited

Loading

scottleibrand Dec 8, 2022

scottleibrand commented Dec 9, 2022

scottleibrand commented Mar 5, 2023

scottleibrand commented Mar 6, 2023

fix(lib/iob): Move value checks up to index.js #1437

fix(lib/iob): Move value checks up to index.js #1437

Conversation

ChanceHarrison commented Oct 23, 2022 • edited Loading

scottleibrand commented Oct 23, 2022

ChanceHarrison commented Oct 23, 2022

mountrcg commented Dec 8, 2022

mountrcg commented Dec 8, 2022

mountrcg commented Dec 8, 2022

mountrcg commented Dec 8, 2022

scottleibrand Dec 8, 2022

Choose a reason for hiding this comment

ChanceHarrison Dec 8, 2022

Choose a reason for hiding this comment

scottleibrand Dec 8, 2022

Choose a reason for hiding this comment

ChanceHarrison Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

scottleibrand Dec 8, 2022

Choose a reason for hiding this comment

scottleibrand commented Dec 9, 2022

scottleibrand commented Mar 5, 2023

scottleibrand commented Mar 6, 2023

ChanceHarrison commented Oct 23, 2022 •

edited

Loading

ChanceHarrison Dec 8, 2022 •

edited

Loading