-
Notifications
You must be signed in to change notification settings - Fork 689
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
Add separate setting for sigmoid adjustment factor #132
Conversation
Brings in nightscout/trio-oref#21 oref0 branch: separate_adjustment_factors - git version: 969586d
Just want a sanity check - changes in open-iaps-oref from here will make their way into open-iaps-oref? |
Everything here looks good and makes sense :) Good work! |
Yes, PR 21 in Oiref does this: nightscout/trio-oref#21 I added you as reviewer :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected after brief testing.
c035a99
to
9a11368
Compare
Tested on simulator. Seems Ok even no information is displayed on the "loop popup" about using sigmoid function. |
Do we want to adress this? |
Here’s my assumption of what’s happening there… I think Sigmoid requires 21+ hours of TDD data in order to be used and if Sigmoid is being used then it won’t show up in the Loop toggle. I’m guessing you tested as a brand new installation in a simulator? I’ll give it a look in a sim in a bit to confirm, though. Unless my assumption is wrong? |
Yes. I tested on a fresh install with less 24h of data. I though there is a info in loop toggle in iAPS but not sure |
Please evaluate if PR needs to be updated or if there is an issue with dev. Test Case:
How does this PR differ from PR#114 which does apply without errors. |
Soo… PR#114 didn’t actually include the minimized oref changes, which is what’s causing the merge conflicts here. I think I’m going to close this PR and just update 114 instead by merging in dev and applying the oref updates. |
Closed since alpha was merged into dev. See #114 for future progress on this PR. |
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.