Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use maxBolus to set automaticDosingIOBLimit #1871

Merged
merged 33 commits into from
Feb 19, 2023
Merged

Use maxBolus to set automaticDosingIOBLimit #1871

merged 33 commits into from
Feb 19, 2023

Conversation

marionbarker
Copy link
Contributor

@marionbarker marionbarker commented Dec 15, 2022

This replaces PR #1870.
This PR adds one safety check so that automatic dosing of insulin cannot exceed a simple ratio multiplied by a constant set in LoopConstants. It addresses the more critical portion of Issue #1863.

If this is deemed a good direction, there are some items to do:

  • add proper logging
  • report in the Delivery Limits section that the maximum of automatic delivery of insulin will be capped by the ratio times maxBolus to make it clear to the user
  • decide what the correct ratio should be (1.2 was entered as a placeholder) - subsequently changed to 2.0.

@marionbarker
Copy link
Contributor Author

I think we might want to revisit this concept and add another Setting for user to adjust.

The reason is there were several people who commented on zulipchat in the last 24 hours that to minimize problems with large boluses, they set their maxBolus small, e.g., 5 U, and let autobolus take them up to the 20 U that their meal needs.

@marionbarker
Copy link
Contributor Author

Increase ratioMaxAutoInsulinOnBoardToMaxBolus to 2.0
Theory of operation:

  • maxBolus is used to limit single doses, but max auto IOB can be higher
  • with Loop in automatic mode (using either dosing strategy), subsequent loops will increase delivery up to ratioMaxAutoInsulinOnBoardToMaxBolus times maxBolus

For users who limit single dosing with smaller maxBolus but need more than 2 times maxBolus as active insulin on board, there may need to be a modification of their current method with this safety feature added.

For example, user who needs 20 U IOB but also needs to limit any given bolus to 5 units to avoid problems with their site:

  • Set maxBolus to 10 (or up to 12.5 U)
  • For meal entry, manually limit delivery to 5 U (or set no delivery and rely on autobolus)
  • Each subsequent autobolus will be no more than 5 U and IOB can reach 20 U (or 25 U).

@marionbarker marionbarker changed the title Use maxBolus and ratio to set maxAutoIOB Use maxBolus and ratio to set automaticDosingIOBLimit Dec 19, 2022
@marionbarker marionbarker changed the title Use maxBolus and ratio to set automaticDosingIOBLimit Use maxBolus to set automaticDosingIOBLimit Dec 19, 2022
@marionbarker
Copy link
Contributor Author

Update some variable names.

See also LoopKit PR 442: Update max bolus information screen

Copy link
Collaborator

@ps2 ps2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I didn't submit this review (it was marked pending). I think this is mostly good, but could be cleaned up a bit (see comments), and this kind of change does deserve unit tests.

Loop/Managers/DoseMath.swift Outdated Show resolved Hide resolved
DoseMathTests/DoseMathTests.swift Outdated Show resolved Hide resolved
Loop/Managers/DoseMath.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@ps2 ps2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this further, and I think handling of IOB needs some updates (see comments).

The changes for DoseMath tests are good, but do not cover LoopDataManager changes. It would be good to see some tests covering the changes in LoopDataManager.

@@ -1647,7 +1650,9 @@ extension LoopDataManager {
lastTempBasal: lastTempBasal,
volumeRounder: volumeRounder,
rateRounder: rateRounder,
isBasalRateScheduleOverrideActive: settings.scheduleOverride?.isBasalRateScheduleOverriden(at: startDate) == true
isBasalRateScheduleOverrideActive: settings.scheduleOverride?.isBasalRateScheduleOverriden(at: startDate) == true,
insulinOnBoard: dosingDecision.insulinOnBoard?.value,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DosingDecision's responsibility is for recording the state of things during a dosing decision. I think use its value as an input here is mixing up responsibilities too much, and will make it hard to extract dosing decision if we change how that's generated. The value should probably be passed to this method (preferred, to move more towards a functional implementation of the Loop algorithm, rather than the current stateful one), or the value should be assigned to a member variable on LDM.

Regardless of how this value gets here, failure to retrieve IOB should now be considered an error, as it is part of the Loop dosing algorithm. I think it was previously being treated as a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary modification made, awaiting review.
Working on LoopDataManagerTests.swift, nothing pushed on this yet.

@marionbarker
Copy link
Contributor Author

Brought the marionbarker wip/max-auto-iob branch in line with dev.
Asked @novalegra for help. She plans to supply this with a PR to my branch.

Copy link
Collaborator

@ps2 ps2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOB should be made a requirement for looping, if we're trying to ensure a clamp. Also, I recommend removal of the file scope IOB variable in tests, and rather specify what's being done in each test.

DoseMathTests/DoseMathTests.swift Outdated Show resolved Hide resolved
@@ -1647,7 +1649,9 @@ extension LoopDataManager {
lastTempBasal: lastTempBasal,
volumeRounder: volumeRounder,
rateRounder: rateRounder,
isBasalRateScheduleOverrideActive: settings.scheduleOverride?.isBasalRateScheduleOverriden(at: startDate) == true
isBasalRateScheduleOverrideActive: settings.scheduleOverride?.isBasalRateScheduleOverriden(at: startDate) == true,
insulinOnBoard: self.insulinOnBoard?.value,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a failure to get insulin on board, then dosing will not be limited. The insulinOnBoard function parameter here should probably be non-optional and the loop fail with an error if it can't be retrieved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I naively thought the code where self.insulinOnBoard is set:

https://github.com/marionbarker/Loop/blob/wip/max-auto-iob/Loop/Managers/LoopDataManager.swift#L1039-L1048

with the switch for result of .failure or .success handles the case where there is a failure to get insulin on board.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking more closely (and finding your earlier comment) - it does seem to be an a warning.
Changing this is beyond what I can do.

Copy link
Collaborator

@novalegra novalegra Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree that it should be non-optional if a manual bolus isn't being recommended. However, this means we'll also need to retrieve IOB when recommending a manual bolus, something that doesn't currently occur, and we won't end up using that data. Alternatively, I'm hesitant to just pass a junk IOB into insulinCorrection even if it won't be used.

Would it work to add the error check (which then would halt the recommendation for automatic dosing) and keep the IOB parameter as an optional for compatibility with manual bolusing? Also open to linking the insulinOnBoard and automaticDosingIOBLimit as one optional tuple at the call site with associated non-optional parameters, since this expresses the intent of limiting or not limiting a little better
(something like dosingLimit: (insulinOnBoard: Double, automaticDosingIOBLimit: Double)?)

@darrelld3
Copy link

darrelld3 commented Jan 5, 2023 via email

@@ -177,6 +174,7 @@ class RecommendTempBasalTests: XCTestCase {

func testNoChangeOverrideActive() {
let glucose = loadGlucoseValueFixture("recommend_temp_basal_no_change_glucose")
let insulinOnBoard = 7.69
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these and all the below 7.69? Seems an oddly specific number, and I don't think it's obvious why from reading the tests. If you're testing some IOB based limiting or trying to test that it's not limiting, then I think setting specific values here, makes sense. But otherwise, maybe we just keep them all at 0 when we're not actually trying to trigger some IOB level specific logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine returning all prior tests back to 0.0 U IOB - I only changed that because I thought you asked me too. Sorry for the misunderstanding.

The tests where the value of the insulinOnBoard needs to vary are the 7 new tests.

Of all the previously existing tests, the highest dose was 2.3 U, so I chose 7.69 U as IOB.
With that value, the XCTAssert for all the existing tests would pass.
I got lazy pasting in the new let insulinOnBoard into each test and at one point 7.69 was in my paste buffer.
I think I added it starting here because we don't have any clamping tests with OverrideActive.

@ps2
Copy link
Collaborator

ps2 commented Jan 5, 2023

Ok, yes, I see that manual bolusing may still make sense to pass in the maxiob and current iob as nil. And I'm on the fence about pairing them up into a tuple. Will leave that up to you, it does seem maybe a little cleaner to pair them up.

@ps2
Copy link
Collaborator

ps2 commented Feb 10, 2023

Reviewing this more, I think that [GlucoseValue].insulinCorrection should be kept as is, and not modified. There are three main users of this method, recommendedAutomaticDose, recommendedTempBasal, and recommendedManualBolus.

recommendedManualBolus does not use this limit, so we're passing in the iob limiting parameters as empty passthroughs (discussed above a bit). The other two do apply IOB based limiting, and because of how they deliver, arguably should be applying the limit differently. For fast boluses, it's probably ok to ignore time, and just consider it immediate, and limit boluses to maxiob - iob. For temp basals, though, we should probably be targeting the calculated IOB at t+30, as we are calculating the amount to deliver over the next 30 minutes.

I'll take a swing at reworking this, maybe not tackling the future IOB target for temp basals, but at least cleaning up the optionals, simplifying the manual basal path, and keeping insulinCorrection intact.

@ps2
Copy link
Collaborator

ps2 commented Feb 10, 2023

Also encountered an error I didn't notice in my initial review: the bolus amount is being limited before the partial application factor is applied. For example, if max bolus is 5, max automatic IOB is 10, IOB is 9.5 and the needed insulin is > 0.5 U, then the insulinCorrection in this PR was limiting to 0.5, and then applying partial application factor, which results in an automatic bolus of 0.2, rather than 0.5 U.

@marionbarker
Copy link
Contributor Author

I'm glad you are looking at this. Happy to do any testing you want.

@ps2
Copy link
Collaborator

ps2 commented Feb 10, 2023

And a longstanding, but probably not too impactful bug: if max bolus, or the partial application max bolus are not a deliverable unit, Loop is not rounding to deliverable units, so we could be issuing unsupported boluses to pumpmanagers.

@novalegra
Copy link
Collaborator

novalegra commented Feb 11, 2023

@ps2 as a heads up - I don't think my PR against @marionbarker's branch for this PR is merged in. I don't think it impacts the partial application factor issue you noted but does clean up the optionals

@ps2
Copy link
Collaborator

ps2 commented Feb 13, 2023

Thanks @novalegra, I do appreciate you taking a swing at the optional issue. I think moving the limit application into the dose calculations for each kind of dosing (rather than the shared insulinCorrection method) obviates the need for that approach. Sorry I didn't realize this earlier and save you that work.

@marionbarker
Copy link
Contributor Author

Regarding #1871 (comment) - I did not know how to approach this a different way.

That's why for all the testing I did, I noted that I artificially set partial bolus application factor to 100% to make is easier to evaluate the test results. I did the same thing in the tests added to the testing suite.

@ps2 ps2 changed the base branch from dev to main February 18, 2023 20:42
@ps2 ps2 changed the base branch from main to dev February 18, 2023 20:43
@ps2 ps2 self-requested a review February 19, 2023 20:08
@alevar1975
Copy link

alevar1975 commented Feb 10, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants