Autotune Code Review

[Suchiman]

I'm using AAPS (xDrip+ Libre2 / Accu-Check Insight -> meaning there are some gotchas) and am a windows user so i tried getting it to run locally on which i gave up after some struggling with WSL dependency conflicts around node-gyp. I then did what every self respecting developer would do and did a line by line port to my favorite and totally unbiased best language 😜 C# and tweaked it until it was bug compatible (producing same output) with AutotuneWeb. During this process i noticed quite a few suspicious code snippets where i was unable to tell if it's an atrocious feature of dynamic code or a bug.

1. This code will only warn, not actually handle the case that glucose data couldn't be found, thus possibly either working with undefined variables or the values of the last loop iteration. https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/autotune-prep/categorize.js#L148-L160
2. Only uses the 0'th element but doesn't pass the currentIOBOnly flag https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/autotune-prep/categorize.js#L209
3. inputs.clock isn't passed by all callers and will be undefined here which tz turns into the current timestamp https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L174
4. Just goes on if duration couldn't be computed https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L350-L352
5. This code doesn't set the timestamp property https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L362-L373 which leads to https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L515 effectively doing new Date(undefined) which results in an Invalid date that returns NaN for its instance methods causing https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/profile/basal.js#L19 nowMinutes to become NaN, thus always failing this condition https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/profile/basal.js#L22 and effectively returning https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/profile/basal.js#L14
6. currentItem.rate is sometimes undefined / null causing netBasalRate to become NaN here https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L546
7. This uses created_at but the other code seems to assume started_at (yaay schemaless) https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L556
8. new Date(0).getTime() is a very elaborate / obfuscated way of saying 0, https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/index.js#L40-L42
9. It's called endOffset here but in the JSON file it's endoffset (although, just thinking about it, that could also be an error from AutotuneWeb) https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/profile/isf.js#L16 https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/profile/isf.js#L41
10. Also this code recomputes endOffset even it's already set https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/profile/isf.js#L30-L38
11. This code checks current._type https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/meal/history.js#L58 which is a property that does not exists in my nightscout data unlike eventType from the quite similiar looking code here https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L294
12. current.insulin is sometimes null here https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L301
13. These two pieces initialize temp.timestamp with current.created_at and then try to find duplicates with current.timestamp that might not exists or be different from created_at https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/meal/history.js#L74-L78 https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/meal/history.js#L87-L89
14. This code uses current.absolute for AAPS but unless you set always use absolute values, which must not be used with insight, AAPS sends rate (with the same value as absolute) https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L317-L319 which is the case for the non AAPS case here https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/history.js#L325-L327 i've personally changed the code to either use absolute ?? rate whatever is present
15. Here's some abuse of the insane scoping rules of JS, declaring variables in an inner scope, then using them in the outer scope of different loop iterations https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/autotune-prep/categorize.js#L250-L252
16. This code just moves on https://github.com/openaps/oref0/blob/acb0150021e7bf284b6741a9510076a13f690747/lib/iob/calculate.js#L112-L114
17. Some places declare variables without ever using var thus creating them in global scope.
18. Some places use number = number.toFixed(precision) to round which actually returns a string and then use that elsewhere to do arithmetic. Also this uses IEEE754 compliant AwayFromZero rounding while other code uses Math.round(x * 1En) / 1En which uses half-up rounding that will also round differently than IEEE754 compliant code since the multiplying fucks with otherwise irrepresentable numbers like 0.15 which is actually 0.1499999999999999944488848768742172978818416595458984375 and thus rounds with toFixed(1) to 0.1 but with Math.round(0.15 * 10) / 10 to 0.2

All in all the lack of static type safety and the schemaless design of nightscout with attempted normalization scattered across the entire codebase makes this code a pain to work with and the inconsistent naming doesn't help either (treatment might refer to different sorts of objects; the various datetime properties that alternate between unix timestamp, iso8601 string or date instance, the type isn't even guaranteed to be consistent with the same name; all styles are used, lower_snake_case, camelCase, PascalCase). I'd suggest normalizing the data into a single format before the call to prepare even. Most of these things were found by the C# compiler telling me off. I'm sure i've also missed another few cases because i've only started documenting these things halfway through. And after these painful discoveries, here's something to laugh

[scottleibrand]

If you (anyone) would like to fix these issues, please open a pull request against oref0 dev. To verify the fixes don’t have unintended consequences, we can run oref0-backtest with and without the fixes and confirm whether it changes insulin dosing behavior.

[Philoul]

@Suchiman 👍 for code review, If you have some advices with my adaptation of Autotune for AAPS (not finished yet) you're wellcome https://github.com/MilosKozak/AndroidAPS/pull/2669 ...

I have 2 other point that surprised me (it's not a deep analysis): oref0/lib/autotune-prep/categorize.js (lines389-395)

    if (opts.categorize_uam_as_basal) {
        console.error("--categorize-uam-as-basal=true set: categorizing all UAM data as basal.");
        basalGlucoseData = basalGlucoseData.concat(UAMGlucoseData);
    } else if (CSFLength > 12) {
        console.error("Found at least 1h of carb absorption: assuming all meals were announced, and categorizing UAM data as basal.");
        basalGlucoseData = basalGlucoseData.concat(UAMGlucoseData);
    } else {

To verify if all meals were announced, check if there is at least 1h of CSF values seems to me very strange (if you have just only one meal recorded, it's very difficult to have less than one hour of CSF data...)

During my tests I found some days with more than 3 hours of UAM data considered as basal (this test is allways true on my side) For my part, I would have chosen something like "less than one hour of UAM data" (line 392)

    } else if (UAMLength < 12) {
        console.error("Found less than 1h of UAM data: assuming all meals were announced, and categorizing UAM data as basal.");
        basalGlucoseData = basalGlucoseData.concat(UAMGlucoseData);

oref0/lib/autotune/index.js (lines 361-364)

        } else {
            deviations += Math.max(0*previousAutotune.min_5m_carbimpact,parseFloat(CSFGlucose[i].deviation));
            mealCarbs = Math.max(mealCarbs, parseInt(CSFGlucose[i].mealCarbs));
        }

What's the idea behind 0*previousAutotune.min_5m_carbimpact?

[scottleibrand]

Some GitHub archaeology shows that the 0*previousAutotune thing was from https://github.com/openaps/oref0/commit/5b3ecb3fc4c5be6f455f1d6066ca8b2729d28f6a "disable min deviation for CSF calculation" - it was a leftover from something I attempted during initial autotune development, and just never got cleaned up.

[Philoul]

But with this modification you also disable negative deviation in calculation...

For categorize_uam_as_basal setting I never see on my tests any impact on results (I always have more than 1 hour of csf data so test is always true, even with 2 or 3 hours of uam data...). Do you think my proposal is a good idea (UAMLength < 12)?