nightscout / Trio

MIT License
46 stars 130 forks source link

Advanced TempTarget slider #241

Open mountrcg opened 1 month ago

mountrcg commented 1 month ago
The current Experimental TT slider has disadvantages. old Slider arguments
Simulator Screenshot - Dev 12 mini - 2024-05-26 at 22 19 27 The slider for the Target Glucose actualy slides HalfBasalExercixeTarget (HBT) and shows the calculated TempTarget with that HBT and the the chosen Insulin fraction above. This will result in above shown example that with a high sensitivity of 75% Insulin one could set a High TT of max 165mg/dL. This bears no justification. Also from my sports activities, which is the major application, I first choose at which target glucose I want to exercise and when at which sensitivity. The refactored slider adjusts the desired Insulin % (sensitivity ratio) and calculates HBT. So here the boundaries are not applied like in the old solution to HBT but to the sensitivity ratio. For highTT the min sensitivity ratio is 15%, for lowTT the autosensMax defines the border of low sensitivity applied by the TT.

New Advanced TT slider:

https://github.com/nightscout/Trio/assets/539276/73477905-623b-4546-adc0-ce1ca0f7fa63

The feature itself is standard oref, but it requires setting an HBT to arrive at desired sensitivity. This is a comfortable solution to use that feature for exercise TT's. The new slider addresses the issues of boundaries and also increments the single slider by 5% points. This solution still uses Jon's oref2 variable injection of HBT into oref as a variable that overrides the HBT as defined in user preferences. This PR could be a good playing ground to change that and make the HBT calculation result the new user preference for the time the TT is active. Once the TT is not active anymore the user preference should revert to the state before TT. Unfortunately I would need assistance with that.

Once this is discussed and implemented it could be also conceptually used for Overrides and how the temporary replace user preferences. This way no additional injections into oref are needed for TT's and Profile Overrides.

mountrcg commented 1 month ago

Test cases for review include

  1. preference lowTTlowersSens is disabled/enabled
  2. preference highTTraisesSens is disabled/enabled
  3. autosensMax is =1 / >1
  4. autsensMin is =1 / <1
  5. change level of HBT in prefs
dnzxy commented 1 month ago

Just a side note: it looks like you had checked out an older version of the MinimedKit submodule in your local branch. Please update (by pulling in latest module SHA) or else this PR would regress the submodule in comparison to dev.

mountrcg commented 1 month ago

rebased on current dev and fixed submodule issue.

mountrcg commented 1 month ago

@dnzxy could you request the reviews, I do not have permission to do this. I do not think the work around the oref2-Variable thing will be done, as I do not have the expertise. But it could go like this, same structure as the Experimental Slider, just without the disadvantages and with a little more information. @dsnallfot it does however need a rework of the edit part you just did for experimental, I tried but could not get it to re-calculate the percentage on edit, nor the save of the new HBT if edited.

dnzxy commented 1 month ago

Reviews requested, thanks for the ping 😊

Regarding your editing issue: Not sure, but I think #236 can be considered a "given", so while I generally do not advise to base features on other features while they are being worked on, I think this PR should be based on 236, as 236 will very likely be merged the next 24-72 hrs and then this PR would regress the functionality that got introduced with 236. @dsnallfot can you help Robert with his feature here?

dsnallfot commented 1 month ago

Reviews requested, thanks for the ping 😊

Regarding your editing issue: Not sure, but I think #236 can be considered a "given", so while I generally do not advise to base features on other features while they are being worked on, I think this PR should be based on 236, as 236 will very likely be merged the next 24-72 hrs and then this PR would regress the functionality that got introduced with 236. @dsnallfot can you help Robert with his feature here?

Of course. I can try to help. But not before the weekend since I'm totally busy at work right now

mountrcg commented 1 month ago

... I think this PR should be based on 236, as 236 will very likely be merged the next 24-72 hrs ...

ok, so will rebase once more when #236 is merged, and then try to fix editing of adv.TT.

Sjoerd-Bo3 commented 1 month ago

... I think this PR should be based on 236, as 236 will very likely be merged the next 24-72 hrs ...

ok, so will rebase once more when #236 is merged, and then try to fix editing of adv.TT.

It is merged :)

mountrcg commented 1 month ago
So I am not lucky to change a Preset with a different HBT, also known as Experimental/Advanced TT. When editing I do not know how to acquire the saved HBT that belongs to that preset. Therefore it uses standard 160mg/dL - see screenshot 1. Also the editing does not save the new entered percentage. Due to this changed slider the percentage does not need to be computetd, it's a direct slider input, but HBT gets computed! See screenshot 2 of edited percentage, however the old 200% will be enacted! I put some comments into the code. @dsnallfot I wait for your return patiently to help - no big deal. Never mind the TempTarget Name - it's actualy a lowTTlowersSens Screenshot 1 Screenshot 2
Error-EditPreset with changed HBT Simulator Screenshot - Dev mini - 2024-05-30 at 17 18 12
mountrcg commented 1 month ago

I left the computePercentage function in there as it would be needed to set the Slider at the appropriate position when editing the Advanced TT, if one could retrieve the proper HBT for this preset. Most probably that function should be moved from FreeAPS/Sources/Modules/AddTempTarget/AddTempTargetStateModel.swift to FreeAPS/Sources/Modules/AddTempTarget/View/AddTempTargetRootView.swift, right?

dsnallfot commented 1 month ago

Sorry for the late reply. Will now test build and tinker with this a bit before the kids wake up in a couple of hours. Get back to you as soon as I have any input.

dsnallfot commented 1 month ago

ok, have not solved anything yet, but can provide a small clue on how you could do to save the preset hbt part at least - I think this would work and not cause issue with NS uploads etc:

Skärmavbild 2024-06-01 kl  11 41 49

Temp targets currently don't use "notes" as an attribute (which other treatments do), but I guess we could add that attribute and save/write/read the hbt in all newly created presets there.

Skärmavbild 2024-06-01 kl  11 52 47

And this means that the "notes" needs to be added in the Temp Target model (and in the temp target entry and updatedPreset and watchManager tempTargetID also).

Skärmavbild 2024-06-01 kl  11 40 34

To be continued later...

dsnallfot commented 1 month ago

Or even better maybe, to introduce a new attribute "hbt" to all temp target presets. Instead of "notes". (I initially thought about notes since that's part of the standard nightscout attributes)

Skärmavbild 2024-06-01 kl  11 57 04
JeremyStorring commented 1 month ago

Are we in love with "Advanced TT"? It's not immediately clear what it is. I think it would be better to try to be clear and concise, can we fit "Advanced Temp Target" or something similar?

mountrcg commented 3 weeks ago

Was away for while on holiday, so it will take a while for me to catch up on this.

mountrcg commented 1 week ago

Are we in love with "Advanced TT"? It's not immediately clear what it is. I think it would be better to try to be clear and concise, can we fit "Advanced Temp Target" or something similar?

We could just do "Advanced" as we are in the Temp Target Screen anyway.

mountrcg commented 1 week ago

Or even better maybe, to introduce a new attribute "hbt" to all temp target presets. Instead of "notes". (I initially thought about notes since that's part of the standard nightscout attributes)

It is stored in coreDate, @dnzxy gave a hint to fetch the coreData object and edit it. Will give it a go with GPT.