Look ahead the full predictive low window - #732
Open
JustMaier wants to merge 1 commit into
Open
Conversation
The forecast series starts at the current glucose value rather than the first future point, so iterating ceil(predictiveMinutes / 5) entries stopped one step short of the requested horizon. A 20-minute setting only reached 15 minutes ahead, and a 15-minute one reached 10. At every setting the dial behaved as though it were one notch lower. Take one more point so the setting matches its label, and raise the alarm forecast cap from 12 points to 13 so the 60-minute maximum is reachable instead of capping at 55. loopHorizonReachesRequestedMinutes is the regression test: the dip sits at index 4, so a 20-minute look-ahead has to reach it and a 15-minute one must not. The other added cases cover bounds safety when the horizon exceeds the published series and pin the existing behaviour of following the longest forecast rather than the first to run out.
JustMaier
requested review from
bjorkert,
codebymini and
marionbarker
as code owners
July 28, 2026 23:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The predictive low alarm looks ahead one 5-minute step less than the setting says, so the dial reads one notch low. A 20-minute setting behaves exactly like a 15-minute one, a 15 like a 10, and so on at every value.
The forecast series that
LowBGConditionwalks starts at the current glucose value, not at the first future point. Both backends build it that way:DeviceStatusLoopstampsprediction[0]atlastLoopTime, and the OpenAPSpredBGscurves all begin at the cycle's ownbg. Iteratingceil(predictiveMinutes / 5)entries from index 0 therefore stops one step early. Taking one more point makes the setting mean what its label says.alarmForecastPointCapneeded the same correction. At 12 points the series reached 55 minutes, so the 60-minute maximum the editor offers could never be satisfied.The change is exactly equivalent to shifting the dial: for every value the UI allows, the corrected look-ahead fires on the same cycles the old code fired on at the next setting up.
ceil(N/5) + 1 == ceil((N+5)/5), and replaying two weeks of real forecast records reproduces it with identical counts. The one exception is 60, which had no old equivalent because of the cap. So nobody gets behaviour the app could not already produce. Anyone who prefers what they had can select one step lower and get it back exactly.That does mean more alerts for a user who leaves the setting alone, and the added ones are markedly less precise than the ones already firing. Replaying two weeks of records, the alerts the change adds were followed by an actual low about 7% of the time against 37% for the alerts that already fired, judged over 30 minutes; widening that judgement window to 45 minutes moves it to 19% against 40%. Both are worth stating, because a far-horizon forecast point fires early and a generous window flatters it. That tradeoff is the whole reason the mislabelling mattered. Someone who chose 20 minutes and was quietly given 15 was never offered the choice.
Background
This came out of comparing a real overnight low against the forecast the pump had already published at the time. The forecast showed the fall about 25 minutes before it happened, and the alarm did not fire until the reading itself crossed the threshold, which the plain low alarm does anyway. At a 20-minute setting the two points where the dip was already visible were never examined. Replaying those records with the corrected horizon moves the first alert from roughly five minutes of warning to roughly thirty, with no change to any threshold or default.
Worth noting for the Trio path: taking the longest of the four forecast curves rather than the shortest is load-bearing here. One curve runs short of 13 points in about 7% of cycles, and which one it is varies, while the longest never does. Using the shortest would silently claw back part of the horizon.
Tests cover the boundary in both directions, a horizon longer than the published series, and the case where one of the four forecasts runs short.
I'm @JustMaier on the Trio Discord if it's easier to discuss there.