Integral retrospective correction #726

ps2 · 2018-05-22T03:27:06Z

PR for #695.

Needs to not have different behaviors dependent on when the app was relaunched.
Need to have better way of visualizing effects.
Does it need to be special-cased for carbs?

dm61 · 2018-06-19T00:06:02Z

@ps2 I've tried to address your comments in the latest update, which includes the following

Upon Loop restart, integral RC states are recomputed based on past 60 minutes of data. I think this is a safe way to restart integral RC as it relies only on LoopKit data storage, and includes all safety features already provided by LoopKit
RC display on Predicted Glucose page now clearly shows all contributions
An on/off switch has been added for integral RC, so the display now looks like this:

I think we need to keep reset of integral action when discrepancy is positive and carb effect exceeds a certain threshold, named carbEffectLimit. This is related to a safety issue in scenarios when carbs entered are overestimated yet are initially absorbing faster than anticipated. In such scenario, integral RC would initially accumulate positive discrepancy leading to more aggressive dosing up front and more likely low on the tail end. I had experienced this in my own early testing of integral RC.

In the latest update, instead of setting carbEffectLimit to a fixed value (equal to modeled carb effect of 1 mg/dL/min), it is now set to a value proportional to user's carb sensitivity (ISF/CR ratio), in these lines.

Kdisimone · 2018-06-21T21:32:36Z

If both sliders on, would that functionally be same as having IRC on? I am a little confused how the toggle isn't more of a "If you want an RC feature enabled, choose between RC and IRC"

dm61 · 2018-06-22T01:01:35Z

@Kdisimone The switches are meant to convey the notion that IRC is an extension of RC, not an alternative to RC. It is possible to enable RC but not IRC, but it is not possible to enable IRC unless RC is enabled. That logic is wired into the two switches: if RC is disabled, this automatically disables both RC and IRC. If IRC is enabled, this automatically enables both RC and IRC.

Kdisimone · 2018-06-22T01:04:05Z

Thanks @dm61 that helps! I'll keep that in mind for docs and fb questions when that comes.

shanselman · 2018-08-03T05:47:36Z

Common/Extensions/NSUserDefaults.swift

@@ -112,7 +112,8 @@ extension UserDefaults {
                    maximumBasalRatePerHour: maximumBasalRatePerHour,
                    maximumBolus: maximumBolus,
                    suspendThreshold: suspendThreshold,
-                    retrospectiveCorrectionEnabled: bool(forKey: "com.loudnate.Loop.RetrospectiveCorrectionEnabled")
+                    retrospectiveCorrectionEnabled: bool(forKey: "com.loudnate.Loop.RetrospectiveCorrectionEnabled"),
+                    integralRetrospectiveCorrectionEnabled: bool(forKey: "com.loopkit.Loop.IntegralRetrospectiveCorrectionEnabled")


Does it matter that one setting is "com.loopkit" and the other is "com.loudnate?"

No, any unique key would do. The keys are following Nate and Pete's naming convention.

shanselman · 2018-08-03T06:00:23Z

FWIW I'm running this patch now as well, sitting on top of 1.5.7dev.

francescaneri · 2018-10-06T17:46:39Z

@dm61 @ps2 you can join the branch?

dm61 · 2018-10-06T18:08:26Z

@francescaneri Pete has added necessary resources to LoopKit and has provided everything needed to update and improve the IRC implementation, which is what I am working on now.

francescaneri · 2018-10-06T18:11:52Z

@dm61 thanks

fix minor errors

dm61 · 2018-10-14T03:58:00Z

Integral retrospective branch has been completely reworked. All credits and thanks go to @ps2 who developed LoopKit resources that enable more robust, safer and accurate implementation of IRC algorithm:

Behavior is consistent across app restarts, just like the rest of Loop.
Any edits in past carb entries or other settings are correctly reflected in IRC calculations
IRC algorithm now interacts with dynamic carb absorption in a way that eliminates the need to disable IRC operation in the presence of more significant carb absorption. This safety feature is now built into the operation without the need for a somewhat artificial threshold parameter (carbEffectLimit in the earlier IRC implementations)

Taking advantage of the new implementation approach, I have also made some tweaks in the IRC algorithm to reduce likelihood of glucose oscillations that may occur if ISF value is on a low side, and to reduce lag time in response to increases in bg after carb treatments of lows. These are relatively small improvements, please do not expect that these issues have been completely resolved. If ISF value is too low, you will likely observe some glucose oscillations with or without RC or IRC.

I've been testing the new IRC for about a week before making it available for testing by others. But, since this is a major rework of IRC code, some careful testing and feedback would be very welcome and much appreciated.

Kdisimone · 2018-10-14T03:59:12Z

@dm61 thanks for the work! After some family stuff early this week, I'd be happy to test!

elnjensen · 2018-10-17T11:05:38Z

I've been running this new IRC code for several days with no issues. I can't do a head-to-head comparison with the old code, but it seems to be working fine. Let me know if there are specific scenarios you'd like me to watch for.

Thanks @dm61 and @ps2 for this overhaul to integrate this into Loop.

ps2

IRC algorithm state should be moved into its own class, and should have unit tests written against it. Also, IRC algorithm factors should be included in an issue report.

ps2 · 2018-10-17T16:16:29Z

Loop/Managers/AnalyticsManager.swift

@@ -118,6 +118,10 @@ final class AnalyticsManager: IdentifiableClass {
        if newValue.retrospectiveCorrectionEnabled != oldValue.retrospectiveCorrectionEnabled {
            logEvent("Retrospective correction enabled change")


RC should be enabled for everyone, and allow IRC to be turned on/off.

Would you prefer to have just the RC slider switch removed (while still keeping retrospectiveCorrectionEnabled so that one could disable RC by code modification), or to have retrospectiveCorrectionEnabled completely removed from Loop?

retrospectiveCorrectionEnabled can be removed. It's still easy to remove from enabledEffects if someone wanted to in the code.

ps2 · 2018-10-17T16:17:27Z

Loop/Managers/LoopDataManager.swift

@@ -40,6 +40,19 @@ final class LoopDataManager {
            NotificationCenter.default.removeObserver(observer)
        }
    }
+
+    // Integral restrospective correction parameters
+    private let currentDiscrepancyGain = 1.0 // Standard retrospective correction gain


IRC algorithm state should be moved into its own class.

Done that. Would be great if you could take a look and let me know if anything else needs be done. Then, I'll look into unit tests and issue report, will likely have questions about how to proceed.

Looking good. The class should be moved into its own file, and that will allow room for some documentation about how it works. Being an important core algorithm, it would be good to spend some time documenting the general functionality of IRC, the functions in the class, their parameters, and any static parameters. https://nshipster.com/swift-documentation/

IRC class is now in a separate file, including more detailed comments. I'll add IRC state to issue report next. Regarding unit tests, I've looked at DoseMathTests and searched around a bit, but I am still not sure how to put the unit tests together. Looks like need to add another target? Can you recommend a good reference (for beginners)?

ps2 · 2018-10-17T16:18:02Z

Loop/Managers/LoopDataManager.swift

+    // Integral restrospective correction parameters
+    private let currentDiscrepancyGain = 1.0 // Standard retrospective correction gain
+    private let persistentDiscrepancyGain = 5.0 // Retrospective correction gain for persistent long-term errors, must be >= currentDiscrepancyGain
+    private let correctionTimeConstant = 120.0 // Retrospective correction filter time constant in minutes


time constants should use TimeInterval

IRC algorithm moved into its own class.

IRC class in a separate file, with comments

add IRC state to issue report

ps2 · 2018-10-25T17:07:57Z

This is looking good, but could be cleaned up a bit more: There should be a generic interface that returns glucose effect, and two implementations: one being your new IRC, and one being a legacy RC. That same interface can store its own “total retrospective correction” value or even generate its own user-facing description.

The length of integration should be included in the issue report, as things like sign changes cause the integration to exit early.

dm61 · 2018-10-28T00:08:48Z

@ps2, I've refactored RC based on the last comment, but I'm not really sure this is what you were looking for. Maybe the RetrospectiveCorrection protocol should be organized a bit differently?

francescaneri · 2018-10-31T21:53:25Z

@dm61 much more stable 👍 good job

dm61 · 2018-10-31T22:21:44Z

@francescaneri thanks for the feedback. Just fyi, I have just a few more things to do to complete IRC implementation, but I've been really swamped at work and other things, which is why this is taking some time.

dm61 · 2018-11-06T18:28:36Z

Updates:

Refactored IRC and RC as two implementations having the same generic retrospective correction interface, which takes glucose and discrepancies and returns glucose effect
Added a set of unit tests for IRC class (it was nice to learn a little bit about unit tests)
Removed retrospectiveCorrectionEnabled and RC slider switch, so standard RC is now always enabled, while IRC can be turned on or off

At this point, I think I've completed all I've planned to do based on the review comments. Of course, would not mind making any further improvements if need be.

francescaneri · 2018-11-09T16:45:05Z

@dm61 I'm testing. I can tell you over the weekend!

francescaneri · 2018-11-15T09:27:38Z

improved: the basal and the algorithm work much better. even the predilection is more realistic. thanks for the job @dm61

…ve-correction

francesc0-cgm · 2019-02-02T11:09:59Z

@ps2 @Kdisimone it is possible to merge it?

dabear · 2019-04-05T07:16:50Z

@ps2 any comments regarding what's needed to get this into Loop proper?

…1.10.0dev

dm61 · 2019-05-11T16:28:01Z

@ps2 I'd like to request closing this PR. As far as I can tell, anecdotal feedback from users who have tried branches with IRC has generally been favorable. However, I do not see how to completely eliminate risks associated with more aggressive corrections, especially in cases when ISF setting is too low. I would therefore recommend that we remove IRC from further considerations for the mainline Loop. To the extent I am able to do so, I am planning to keep the IRC branch up to date with Loop dev, so this feature will remain available for interested users.

I'd like to thank you for providing LoopKit resources in support of IRC. I have no doubts these resources will prove beneficial in future Loop algorithm developments. Finally, I can't thank you enough for keeping high bar for Loop in terms of safety, effectiveness, user experience, and code quality.

scottleibrand · 2019-05-11T19:13:13Z

LMK if you or anyone else want to try an approach along the lines of autosens: IRC appears to be solving much the same problem, of sensitivity running higher or lower than normal for an extended period. As best I can tell autosens doesn’t have the same safety concerns that you mention here with regard to exacerbating poor ISF settings.

francescaneri · 2019-05-11T19:32:49Z

with overrides-mode the integration would be simpler 👍

ps2 · 2019-05-18T14:59:57Z

I think we might still want to include some of the core computations of this work; and also do more analysis that helps us understand what conditions it works in and when it doesn't. But I'll close it for now.

dm61 mentioned this pull request May 22, 2018

Integral retrospective correction #695

Closed

shanselman reviewed Aug 3, 2018

View reviewed changes

IRC-discrepancy-timeline

7009e32

fix minor errors

dm61 force-pushed the integral-retrospective-correction branch from 5888513 to 34c390a Compare October 14, 2018 02:22

ps2 commented Oct 17, 2018

View reviewed changes

dm61 added 6 commits October 20, 2018 22:36

IRC as separate class

4311919

Merge pull request #8 from dm61/IRC-class

d7a17fe

IRC algorithm moved into its own class.

IRC class in a separate file, with comments

9eae122

Merge pull request #9 from dm61/IRC-class

b653a7c

IRC class in a separate file, with comments

IRC issue report

d774f4c

Merge pull request #10 from dm61/IRC-class

b2beb20

add IRC state to issue report

refactor RC to generic protocol

17468f1

reorganize RC protocol

d3a4d06

dm61 added 2 commits October 31, 2018 21:44

settings moved to RC/IRC calcs

5bb4eab

IRC unit tests

ca9bd27

standard retrospective correction always enabled

908d755

dm61 added 2 commits December 20, 2018 12:47

Merge remote-tracking branch 'upstream/dev' into integral-retrospecti…

2649cee

…ve-correction

Merge branch 'IRC-backup' into integral-retrospective-correction

6ce3673

mitchellhenke mentioned this pull request Apr 1, 2019

Fpu dev #849

Closed

dm61 added 3 commits April 21, 2019 09:50

Update project.pbxproj

ed12f3c

Merge branch 'integral-retrospective-correction' into IRC-rebase-to-v…

aa2140b

…1.10.0dev

update IRC code to v1.10.0dev

1b8cecd

dm61 force-pushed the integral-retrospective-correction branch from 6ce3673 to 1a96620 Compare April 24, 2019 23:46

ps2 closed this May 18, 2019

		@@ -118,6 +118,10 @@ final class AnalyticsManager: IdentifiableClass {
		if newValue.retrospectiveCorrectionEnabled != oldValue.retrospectiveCorrectionEnabled {
		logEvent("Retrospective correction enabled change")

Integral retrospective correction #726

Integral retrospective correction #726

Conversation

ps2 commented May 22, 2018 • edited Loading

dm61 commented Jun 19, 2018

Kdisimone commented Jun 21, 2018

dm61 commented Jun 22, 2018

Kdisimone commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanselman commented Aug 3, 2018

francescaneri commented Oct 6, 2018

dm61 commented Oct 6, 2018 • edited Loading

francescaneri commented Oct 6, 2018

dm61 commented Oct 14, 2018

Kdisimone commented Oct 14, 2018 • edited Loading

elnjensen commented Oct 17, 2018

ps2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ps2 commented Oct 25, 2018

dm61 commented Oct 28, 2018

francescaneri commented Oct 31, 2018 • edited Loading

dm61 commented Oct 31, 2018

dm61 commented Nov 6, 2018 • edited Loading

francescaneri commented Nov 9, 2018

francescaneri commented Nov 15, 2018

francesc0-cgm commented Feb 2, 2019

dabear commented Apr 5, 2019

dm61 commented May 11, 2019

scottleibrand commented May 11, 2019

francescaneri commented May 11, 2019

ps2 commented May 18, 2019

ps2 commented May 22, 2018 •

edited

Loading

dm61 commented Oct 6, 2018 •

edited

Loading

Kdisimone commented Oct 14, 2018 •

edited

Loading

francescaneri commented Oct 31, 2018 •

edited

Loading

dm61 commented Nov 6, 2018 •

edited

Loading