Add separate setting for sigmoid adjustment factor #114
Conversation
|
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. |
|
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 |
MikePlante1
changed the title
|
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. |
|
I wouldn't want to scrap Sigmoid. I like the adjustability and profile ISF compatibility (outside of the scope here though :-) ) |
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 |
Brings in nightscout/trio-oref#21 oref0 branch: separate_adjustment_factors - git version: 969586d
|
Successful test that patch appears to works for alpha. The same patch files work for alpha as well as dev branch:
|
and brings in "tddAdjBasal pop-up correction" nightscout/trio-oref#22 oref0 branch: dev - git version: fa373c9 Last commits: fa373c9 Merge pull request 22 from nightscout/tmhastings-tddAdjBasal fc0ae69 tddAdjBasal pop-up correction 487bcbb Merge pull request 21 from MikePlante1/separate_adjustment_factors
Merge in dev and update oref files
|
Thanks @bjornoleh for the PR. I built this to my real looping phone, previously running alpha, and I noticed everything in the |
There was a problem hiding this comment.
Tested and works as intended.
MikePlante1
requested review from
Sjoerd-Bo3 and
JeremyStorring
and removed request for
mountrcg and
dnzxy
|
I tested this again on a test phone with the new commits. (Prior comment was for a version that not have all the commits).
|
There was a problem hiding this comment.
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. |
Merging this now with the warning above about preferences being reset even building after this merge. |
Marked as Draft until oref changes from nightscout/trio-oref#21 are merged and the minified files replacements are added to this PR or branch.
Addresses issue #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.