nightscout / Trio

MIT License
64 stars 259 forks source link

Add separate setting for sigmoid adjustment factor #114

Closed MikePlante1 closed 3 months ago

MikePlante1 commented 4 months ago

Marked as Draft until oref changes from https://github.com/nightscout/open-iaps-oref/pull/21 are merged and the minified files replacements are added to this PR or branch.

Addresses issue https://github.com/nightscout/Open-iAPS/issues/106 by using separate variables for Adjustment Factor when using Logarithmic DynamicISF vs Sigmoid DynamicISF. Defaults AF to 0.8 for Logarithmic and 0.5 for Sigmoid.

This adds a new setting for Adjustment Factor so Logarithmic DynamicISF can default to 0.8 and Sigmoid DynamicISF can default to 0.5. This prevents new users from starting DynamicISF in Logarithmic mode with a default of 0.5 which seems to often lead to prolonged highs for many people. Also helpful for those who switch between Logarithmic and Sigmoid depending on their situation.

Screenshot 2024-04-14 at 10 10 05 AM
MikePlante1 commented 4 months ago

I tested it more thorougly and everything seems to work as expected on both an iPhone 15 simulator (iOS 17.4) and on a physical testing iPhone SE 2nd gen (iOS 16.2). Both tests used a Pump and Glucose Simulator. Both had been running for over 24 hrs off and on so they had >24 hours of TDD data.

Checking with both Sigmoid toggled on and then off and leaving the default values, theAutosens ratios in the popup display exactly matched the expected values from plugging the variables into the Desmos graph for Dynamic ISF. Updating the Adjustment Factor and Sigmoid Adjustment Factor in Preferences also produced the expected Autosens ratios.

avouspierre commented 4 months ago

If correctly understood, the (log) adjustement factor is not used when sigmoid is selected. Perhaps deactivate the adjustement factor when sigmoid function toggle. And idem for sigmoid adjustement factor only to display when sigmoid is toggled On.

MikePlante1 commented 4 months ago

If correctly understood, the (log) adjustement factor is not used when sigmoid is selected. Perhaps deactivate the adjustement factor when sigmoid function toggle. And idem for sigmoid adjustement factor only to display when sigmoid is toggled On.

This could be something to consider post-v1.0.0 but since the Preferences menu is dynamically generated it's not a simple change. Post-v1.0.0 release, I think the entire Preferences menu should get an overhaul.

dnzxy commented 4 months ago

How about picking the dynamic menu from current iAPS, making adjustments there (add that distinct AF for Sig) and also incorporate Pierre‘s suggestion. Subsequently, you could remove those settings there from the "Open APS" settings, as they aren’t really openAPS ones.

Just thinking out loud here.

My personal 2 cents: there shouldn’t even be a Signoid curve dynISF.

bjornoleh commented 4 months ago

I wouldn't want to scrap Sigmoid. I like the adjustability and profile ISF compatibility (outside of the scope here though :-) )

flyingpie101 commented 4 months ago

How about picking the dynamic menu from current iAPS, making adjustments there (add that distinct AF for Sig) and also incorporate Pierre‘s suggestion. Subsequently, you could remove those settings there from the "Open APS" settings, as they aren’t really openAPS ones.

Just thinking out loud here.

My personal 2 cents: there shouldn’t even be a Signoid curve dynISF.

Out of curiosity, with a statistically relevant population of users using sigmoid, why shouldnt there be Sigmoid ?

dnzxy commented 4 months ago

How about picking the dynamic menu from current iAPS, making adjustments there (add that distinct AF for Sig) and also incorporate Pierre‘s suggestion. Subsequently, you could remove those settings there from the "Open APS" settings, as they aren’t really openAPS ones. Just thinking out loud here. My personal 2 cents: there shouldn’t even be a Signoid curve dynISF.

Out of curiosity, with a statistically relevant population of users using sigmoid, why shouldnt there be Sigmoid ?

Not relevant here, feel free to bring it up on Discord ☺️

marionbarker commented 3 months ago

Successful test that patch appears to works for alpha.

The same patch files work for alpha as well as dev branch:

MikePlante1 commented 3 months ago

Thanks @bjornoleh for the PR.

I built this to my real looping phone, previously running alpha, and I noticed everything in the preferences menu was reset to defaults.

marionbarker commented 3 months ago

I tested this again on a test phone with the new commits. (Prior comment was for a version that not have all the commits).

before-after-pr-114-fresh-build

MikePlante1 commented 3 months ago

I did a code review and this appears correct, but I have one question:

  • In open-iaps-oref/lib/determine-basal/determine-basal.js, I see reference to 24h_14d average
  • There was a change (in logging?) from 7 day average to 10 day average in open-iaps-oref/lib/profile/index.js
  • I saw some conversation going past about this - just pointing this out so the folks that know can say thumbs up or down.

That's kinda outside the scope of this PR, but it seems that iAPS doesn't use 14 days of TDD history but rather just 10 days. It probably would make sense to rename the variables, but ultimately, I'd like to change DynamicISF to be more in line with how AAPS implements it, but I think that should be a post v1 PR since it seems this is how its been in iAPS for a while now, even if the variable names are wrong.

bjornoleh commented 3 months ago

Tested and works as intended.

⚠️Please note, as @MikePlante1 said above, Preferences will be reset when building this branch, or building dev after this is merged. Please screenshot your settings before building!

Merging this now with the warning above about preferences being reset even building after this merge.